-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix a bug in JenningsLieAlgebra and PCentralLieAlgebra #2085
Fix a bug in JenningsLieAlgebra and PCentralLieAlgebra #2085
Conversation
79c8d90
to
492d0cd
Compare
Codecov Report
@@ Coverage Diff @@
## master #2085 +/- ##
==========================================
+ Coverage 69.31% 69.31% +<.01%
==========================================
Files 491 491
Lines 258013 257960 -53
==========================================
- Hits 178841 178814 -27
+ Misses 79172 79146 -26
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't tests use START_TEST
and STOP_TEST
? My guess was that they should for testinstall/standard, but not for testbugfix. Maybe jennings.txt
should go into the bugfix directory with appropriate name change (as it tests a fix, does not provide a comprehensive test coverage for some area of GAP functionality?)
Side remark: there are some other tests in testinstall which miss START_TEST
and STOP_TEST
.
I don't know about any rules that say START_TEST/STOP_TEST must be there. Any particular reason we'd want it here? I don't care where this new test file is put; I only made this PR to help get that fix in. If you want me to move it, I am fine with doing that, I just don't want to move it back again for the next reviewer. :-) |
@fingolfin Lines 1027 to 1034 in 286b995
Keeping calls to (IIRC there was a Wiki page "How to add a test to GAP", but can't seem to be able to find it now). If you want to keep it in testinstall, I suggest to name it |
492d0cd
to
8179a7e
Compare
Alex, I know all that about START_TEST but fail to see the relevance of any of that in this particular case. I have now moved the .tst file to testbugfix. |
OK, then please just rename it to |
Fix a bug reported by Laurent Bartholdi.
8179a7e
to
b8df408
Compare
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! waiting for macos travis build to pass.
Added to release notes for GAP 4.10 at https://github.com/gap-system/gap/wiki/GAP-4.10-release-notes. I've checked what happens in GAP 4.8.10:
so this is classified as a bug that can lead to incorrect results. |
Fix a bug reported by Laurent Bartholdi.
This is a rebased, squashed and slightly improved version of PR #2079