Skip to content
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

Change Test() to reject unexpected or invalid inputs #2519

Merged
merged 1 commit into from
Jun 7, 2018

Conversation

fingolfin
Copy link
Member

This is a somewhat experimental change to fix #2506 -- let's see if the GAP test suite passes with it.

Assuming this works for the test suite, I wonder if there are any reasonable scenarios where the new Error call is triggered and where this is undesirable. If anybody knows such an example, please bring it up. In that case, we could add an option to turn off that error; or restrict the cases in which it is triggered.

@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: tests issues or PRs related to tests labels Jun 6, 2018
@codecov
Copy link

codecov bot commented Jun 6, 2018

Codecov Report

Merging #2519 into master will increase coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2519      +/-   ##
==========================================
+ Coverage   74.41%   74.46%   +0.04%     
==========================================
  Files         481      481              
  Lines      242659   245614    +2955     
==========================================
+ Hits       180586   182907    +2321     
- Misses      62073    62707     +634
Impacted Files Coverage Δ
lib/test.gi 61.66% <0%> (-0.15%) ⬇️
src/io.c 59.88% <0%> (-13.08%) ⬇️
src/opers.c 92.1% <0%> (-1.91%) ⬇️
src/code.c 92.85% <0%> (-0.87%) ⬇️
src/calls.c 94.24% <0%> (-0.43%) ⬇️
src/hpc/tls.h 66.66% <0%> (ø) ⬆️
src/trans.c 99.81% <0%> (ø) ⬆️
src/pperm.c 97.9% <0%> (+0.01%) ⬆️
src/integer.c 92.24% <0%> (+0.04%) ⬆️
src/rational.c 96.87% <0%> (+0.05%) ⬆️
... and 11 more

@ChrisJefferson
Copy link
Contributor

I suggest we merge this, and then see if any packages fail

Copy link
Member

@frankluebeck frankluebeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, that there is probably some proper error if this is triggered.

Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Cf. also #2524 which is somehow complementary to this.

@fingolfin fingolfin merged commit 07f5dce into gap-system:master Jun 7, 2018
@fingolfin fingolfin deleted the mh/strict-test branch June 7, 2018 09:50
@olexandr-konovalov olexandr-konovalov added the gapdays2018-spring Issues and PRs that arose at http://www.gapdays.de/gap-jupyter-days2018 label Jun 8, 2018
@fingolfin fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Sep 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gapdays2018-spring Issues and PRs that arose at http://www.gapdays.de/gap-jupyter-days2018 kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes topic: tests issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Print an alert when trying to test a non-test file
4 participants