Test Error improvements #2531
Merged
Test Error improvements #2531
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fingolfin
reviewed
Jun 8, 2018
lib/test.gi
Outdated
| lines[i][1] = '#' do | ||
| # ignore comment lines and empty lines at beginning of file | ||
| while i <= Length(lines) and | ||
| ((Length(lines[i]) > 0 and lines[i][1] = '#') or lines[i] = "") |
There was a problem hiding this comment.
Simpler expresdion: Length(lines[i]) = 0 or lines[i][1] = '#'
There was a problem hiding this comment.
Fixed (also the failing test)
ChrisJefferson
force-pushed
the
test-warning-2
branch
from
05a484d
to
0762956
Compare
Codecov Report
@@ Coverage Diff @@
## master #2531 +/- ##
=========================================
+ Coverage 74.4% 75% +0.6%
=========================================
Files 481 481
Lines 243488 252048 +8560
=========================================
+ Hits 181157 189047 +7890
- Misses 62331 63001 +670
|
olexandr-konovalov
added
the
gapdays2018-spring
ChrisJefferson
force-pushed
the
test-warning-2
branch
from
0762956
to
8c6ea35
Compare
fingolfin
reviewed
Jun 10, 2018
lib/test.gi
Outdated
| @@ -20,19 +20,18 @@ InstallGlobalFunction(ParseTestInput, function(str, ignorecomments) | |||
| ign := []; | |||
| i := 1; | |||
| while i <= Length(lines) do | |||
| if i = 1 and Length(lines[1]) > 0 and lines[1][1] = '#' then | |||
| if i = 1 and (lines[1] = "" or lines[1][1] = '#') then | |||
There was a problem hiding this comment.
Note that this line creates a temporary empty string object each time it is executed, while the old version with Length does not create any temporaries.
There was a problem hiding this comment.
Usually I'd assume function calls cost, but I agree that Length on strings is cheap. I don't really care about one string, but I have made the change so I can get this merged.
fingolfin
approved these changes
Jun 10, 2018
ChrisJefferson
force-pushed
the
test-warning-2
branch
from
8c6ea35
to
7dd3723
Compare
ChrisJefferson
force-pushed
the
test-warning-2
branch
from
7dd3723
to
13c7dd4
Compare
fingolfin
added
the
release notes: added
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes two changes to #2519 .
Firstly, we produce a better error message, so we know where the problem is.
Secondly, add to the definition of Test files the option of having blank lines at the start of tests. This wasn't technically legal before, but it was used in various tests (including the Ferret package) and was silently accepted.