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

Fix Test to correctly handle test outputs which do not end with a newline when using rewriteToFile #3695

Merged
merged 1 commit into from
Oct 15, 2019

Conversation

ChrisJefferson
Copy link
Contributor

Given a statement in a .tst file which does not end in a newline, rewriteToFile gets very confused, and in particular can "lose" lines of output if run over and over again. This just prints a warning, and inserts a newline.

As an example of the problem, consider this test file:

gap> Print(x -> x);
gap> 1;
gap> 2;
gap> 3;

Running test with rewriteToFile gives:

gap> Print(x->x);
function ( x )
    return x;
endgap> 1;
1
gap> 2;
2
gap> 3;
3

Running again on this gives the following, where the 1 has been lost

gap> Print(x->x);
function ( x )
    return x;
endgap> 2;
2
gap> 3;
3

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 84.476% when pulling 1774013 on ChrisJefferson:fix-rewriteBug into 7a7f72b on gap-system:master.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

I have not actually tested this PR, but the description seems useful, and the code looks plausible to me. Thanks!

@@ -633,6 +633,12 @@ InstallGlobalFunction("Test", function(arg)
od;
if PositionSublist(pf.inp[i], "STOP_TEST") <> 1 then
Append(new[pf.pos[i]], pf.cmp[i]);
if pf.cmp[i] <> "" and Last(pf.cmp[i]) <> '\n' then
Copy link
Member

Choose a reason for hiding this comment

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

Or you could do this, and then we could backport this to stable-4.11 (however, I am perfectly fine with merging this as-is):

Suggested change
if pf.cmp[i] <> "" and Last(pf.cmp[i]) <> '\n' then
if pf.cmp[i] <> "" and not EndsWith(pf.cmp[i], "\n") then

Copy link
Member

@fingolfin fingolfin Oct 9, 2019

Choose a reason for hiding this comment

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

Question (just to check my understanding of the code, which is limited): When is pf.cmp[i] an empty string? If a command is expected to not output anything, maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeps, tests with no output will have pf.cmp[i] empty. This won't cause a problem as we will just start outputting the next input on the line after the previous input. It's just when the line is non-empty but not \n terminated.

@fingolfin
Copy link
Member

@ChrisJefferson feel free to merge this at any time, though it'd be nice if you could set at least the appropriate "release notes" label (and if you deem this should be added to release notes, also make sure the PR title can be copied verbatim to release notes for users). Thanks!

@ChrisJefferson ChrisJefferson added kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Oct 14, 2019
@ChrisJefferson ChrisJefferson changed the title Handle test outputs which do not end in a newline when using rewriteToFile Fix isse with test outputs which do not end with a newline when using the rewriteToFile option of Test Oct 14, 2019
@ChrisJefferson
Copy link
Contributor Author

Added labels, tweaked titles. I don't think this really needs backporting, as it is a very long-standing issue, and (hopefully) 4.11 should only get required fixes now really?

@fingolfin fingolfin changed the title Fix isse with test outputs which do not end with a newline when using the rewriteToFile option of Test Fix issue with test outputs which do not end with a newline when using the rewriteToFile option of Test Oct 15, 2019
@fingolfin fingolfin merged commit bedf46e into gap-system:master Oct 15, 2019
@ChrisJefferson ChrisJefferson deleted the fix-rewriteBug branch December 2, 2019 14:30
@PaulaHaehndel PaulaHaehndel self-assigned this Feb 17, 2021
@PaulaHaehndel PaulaHaehndel added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Feb 17, 2021
@PaulaHaehndel PaulaHaehndel removed their assignment Feb 17, 2021
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them and removed kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them labels Aug 17, 2022
@fingolfin fingolfin changed the title Fix issue with test outputs which do not end with a newline when using the rewriteToFile option of Test Fix Test to correctly handle test outputs which do not end with a newline when using rewriteToFile Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants