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.

Sign up for GitHub

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 bug in Test with rewriteToFile parameter set when applied to mixed input and output test case. #5686

Merged
merged 1 commit into from
Mar 27, 2024
Merged

Fix bug in Test with rewriteToFile parameter set when applied to mixed input and output test case. #5686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions lib/test.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,9 +718,13 @@
od;
for i in [1..Length(pf.inp)] do
new[pf.pos[i]] := "";
for j in [1..Number(pf.inp[i], c-> c = '\n')] do
Append(new[pf.pos[i]], lines[pf.pos[i]+j-1]);
Add(new[pf.pos[i]], '\n');
for j in [1 .. Number(pf.inp[i], c -> c = '\n') +
Number(pf.outp[i], c -> c = '\n')] do
if (j = 1 and StartsWith(lines[pf.pos[i] + j - 1], "gap> ")) or
(j > 1 and StartsWith(lines[pf.pos[i] + j - 1], "> ")) then
Append(new[pf.pos[i]], lines[pf.pos[i] + j - 1]);
Add(new[pf.pos[i]], '\n');
fi;

Check warning on line 727 in lib/test.gi

View check run for this annotation

Codecov / codecov/patch

lib/test.gi#L721-L727 

Added lines #L721 - L727 were not covered by tests
Comment on lines +721 to +727
Copy link
Member

Choose a reason for hiding this comment

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

Huh, Codecov complains that this code is not covered by tests. But your new .tst file should ensure it is now covered? Weird.

Some musings, just to verify I understand this code right (I am not actually requesting a code change): So this code block basically tries to carefully stitch together the input (and now output) of "block i" to recover the input as is in the .ts file. My first naive thought was, "can't we just do this:"

   new[pf.pos[i]] := pf.inp[i];

but that won't work because we stripped all the prompts from pf.inp[i]. Maybe there are other blockers? If this is the only reason, then an alternative solution would be to add a pf.inp_raw field which is created by ParseTestInput in parallel to pf.inp, but instead of e.g.

      Add(inp, lines[i]{[6..Length(lines[i])]});

one would do

      Add(inp_raw, lines[i]);

Yet another might be to do this?

Suggested change
for j in [1 .. Number(pf.inp[i], c -> c = '\n') +
Number(pf.outp[i], c -> c = '\n')] do
if (j = 1 and StartsWith(lines[pf.pos[i] + j - 1], "gap> ")) or
(j > 1 and StartsWith(lines[pf.pos[i] + j - 1], "> ")) then
Append(new[pf.pos[i]], lines[pf.pos[i] + j - 1]);
Add(new[pf.pos[i]], '\n');
fi;
# write out original input, but with prompts restored
for j in [1 .. Length(pf.inp[i])] do
if j = 1 then
Append(new[pf.pos[i]], "gap> ");
else
Append(new[pf.pos[i]], "> ");
fi;
Append(new[pf.pos[i]], pf.inp[i][j]);
Add(new[pf.pos[i]], '\n');

or maybe this

Suggested change
for j in [1 .. Number(pf.inp[i], c -> c = '\n') +
Number(pf.outp[i], c -> c = '\n')] do
if (j = 1 and StartsWith(lines[pf.pos[i] + j - 1], "gap> ")) or
(j > 1 and StartsWith(lines[pf.pos[i] + j - 1], "> ")) then
Append(new[pf.pos[i]], lines[pf.pos[i] + j - 1]);
Add(new[pf.pos[i]], '\n');
fi;
# write out original input, but with prompts restored
Append(new[pf.pos[i]], "gap> ");
Append(new[pf.pos[i]], pf.inp[i][1]);
Add(new[pf.pos[i]], '\n');
for j in [2 .. Length(pf.inp[i])] do
Append(new[pf.pos[i]], "> ");
Append(new[pf.pos[i]], pf.inp[i][j]);
Add(new[pf.pos[i]], '\n');

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the comments! I am also not quite sure what is up with the code coverage, could it be that this is related to the test writing to an external file as opposed to doing the checks completely within the test file itself?

About the different approaches to solving the problem: I was mainly looking to modify the existing code as little as possible. Since the function originally does

Append(new[pf.pos[i]], lines[pf.pos[i] + j - 1]);
Add(new[pf.pos[i]], '\n');

taking the lines from the original input, it seemed prudent to keep this logic as-is and modify the looping logic to select the correct lines instead.

As you point out, pf.inp[i] cannot be used by itself precisely because the prompts get stripped. Furthermore, if I understood the underlying code in ParseTestInput correctly, then pf.inp[i] is actually a string obtained as the concatenation of all input lines separated by newlines \n, not a list of lines, see line 128 of test.gi. Hence the number of input lines is actually Number(pf.inp[i], c -> c = '\n'), which is what Test used prior to my changes. This is the second blocker. So your suggestions won't work because the iterator goes over the number of letters in the input, not the number of lines. To patch it up, however, we could of course first split pf.inp[i] along newlines and then perform the iteration as you suggest. But this seems like a lot of string splitting and concatenating, when in some sense we already have these lines in the lines list.

To me it also makes sense to preserve the existing lines as opposed to trying to build up a new line from the parsed input. The latter is more prone to errors and unintended modifications to the original code, whereas taking lines from the file directly will at worst omit or unintentionally substitute some existing lines for others.

od;
if PositionSublist(pf.inp[i], "STOP_TEST") <> 1 then
Append(new[pf.pos[i]], pf.cmp[i]);
Expand Down
332 changes: 332 additions & 0 deletions tst/testbugfix/2024-03-20-Test-rewriteToFile-option.tst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,332 @@
# Fix issue with `rewriteToFile` option in test cases with mixed input/output
# See https://github.com/gap-system/gap/issues/5685
gap> temp_dir := DirectoryTemporary();;
gap> FileString(Filename(temp_dir, "test_in_001.tst"), \
> """gap> if true then
> > Print("true\n");
> > else
> > Print("false\n");
> > fi;
> true
> """);;
gap> Test(Filename(temp_dir, "test_in_001.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_001.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_001.tst")) = \
> """gap> if true then
> > Print("true\n");
> > else
> > Print("false\n");
> > fi;
> true
> """;
true
gap> temp_dir := DirectoryTemporary();;
gap> FileString(Filename(temp_dir, "test_in_002.tst"), \
> """gap> if true then
> > Print("true\n");
> true
> > else
> > Print("false\n");
> > fi;
> """);;
gap> Test(Filename(temp_dir, "test_in_002.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_002.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_002.tst")) = \
> """gap> if true then
> > Print("true\n");
> > else
> > Print("false\n");
> > fi;
> true
> """;
true
gap> FileString(Filename(temp_dir, "test_in_003.tst"), \
> """gap> if true then
> true
> > Print("true\n");
> > else
> > Print("false\n");
> > fi;
> """);;
gap> Test(Filename(temp_dir, "test_in_003.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_003.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_003.tst")) = \
> """gap> if true then
> > Print("true\n");
> > else
> > Print("false\n");
> > fi;
> true
> """;
true
gap> FileString(Filename(temp_dir, "test_in_004.tst"), \
> """gap> if true then
> > Print("true\n");
> > else
> true
> > Print("false\n");
> > fi;
> """);;
gap> Test(Filename(temp_dir, "test_in_004.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_004.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_004.tst")) = \
> """gap> if true then
> > Print("true\n");
> > else
> > Print("false\n");
> > fi;
> true
> """;
true
gap> FileString(Filename(temp_dir, "test_in_005.tst"), \
> """gap> if true then
> > Print("true\n");
> > else
> > Print("false\n");
> true
> > fi;
> """);;
gap> Test(Filename(temp_dir, "test_in_005.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_005.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_005.tst")) = \
> """gap> if true then
> > Print("true\n");
> > else
> > Print("false\n");
> > fi;
> true
> """;
true
gap> FileString(Filename(temp_dir, "test_in_006.tst"), \
> """gap> if true then
> > Print("true\nfail\n");
> > else
> > Print("false\n");
> > fi;
> true
> fail
> """);;
gap> Test(Filename(temp_dir, "test_in_006.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_006.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_006.tst")) = \
> """gap> if true then
> > Print("true\nfail\n");
> > else
> > Print("false\n");
> > fi;
> true
> fail
> """;
true
gap> FileString(Filename(temp_dir, "test_in_007.tst"), \
> """gap> if true then
> > Print("true\nfail\n");
> true
> > else
> > Print("false\n");
> > fi;
> fail
> """);;
gap> Test(Filename(temp_dir, "test_in_007.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_007.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_007.tst")) = \
> """gap> if true then
> > Print("true\nfail\n");
> > else
> > Print("false\n");
> > fi;
> true
> fail
> """;
true
gap> FileString(Filename(temp_dir, "test_in_008.tst"), \
> """gap> if true then
> > Print("true\nfail\n");
> true
> > else
> > Print("false\n");
> fail
> > fi;
> """);;
gap> Test(Filename(temp_dir, "test_in_008.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_008.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_008.tst")) = \
> """gap> if true then
> > Print("true\nfail\n");
> > else
> > Print("false\n");
> > fi;
> true
> fail
> """;
true
gap> FileString(Filename(temp_dir, "test_in_009.tst"), \
> """gap> if true then
> > Print("true\nfail\n");
> true
> fail
> > else
> > Print("false\n");
> > fi;
> """);;
gap> Test(Filename(temp_dir, "test_in_009.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_009.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_009.tst")) = \
> """gap> if true then
> > Print("true\nfail\n");
> > else
> > Print("false\n");
> > fi;
> true
> fail
> """;
true
gap> FileString(Filename(temp_dir, "test_in_010.tst"), \
> """gap> if true then
> true
> > Print("true\nfail\n");
> fail
> > else
> > Print("false\n");
> > fi;
> """);;
gap> Test(Filename(temp_dir, "test_in_010.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_010.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_010.tst")) = \
> """gap> if true then
> > Print("true\nfail\n");
> > else
> > Print("false\n");
> > fi;
> true
> fail
> """;
true
gap> FileString(Filename(temp_dir, "test_in_011.tst"), \
> """gap> if true then
> true
> fail
>
> > Print("true\nfail\n\n");
> > else
> > Print("false\n");
> > fi;
> """);;
gap> Test(Filename(temp_dir, "test_in_011.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_011.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_011.tst")) = \
> """gap> if true then
> > Print("true\nfail\n\n");
> > else
> > Print("false\n");
> > fi;
> true
> fail
>
> """;
true
gap> FileString(Filename(temp_dir, "test_in_012.tst"), \
> """gap> if true then
> true
> fail
>
> > Print("true\nfail\n\n");
> > else
> > Print("false\n");
> > fi;
> gap> Print("Hello\nWorld\n");
> Hello
> World
> """);;
gap> Test(Filename(temp_dir, "test_in_012.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_012.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_012.tst")) = \
> """gap> if true then
> > Print("true\nfail\n\n");
> > else
> > Print("false\n");
> > fi;
> true
> fail
>
> gap> Print("Hello\nWorld\n");
> Hello
> World
> """;
true
gap> FileString(Filename(temp_dir, "test_in_013.tst"), \
> """gap> if true then
> true
> fail
>
> > Print("true\nfail\n\n");
> > else
> > Print("false\n");
> > fi;
>
> # New test case
> gap> Print("Hello\nWorld\n");
> Hello
> World
> """);;
gap> Test(Filename(temp_dir, "test_in_013.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_013.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_013.tst")) = \
> """gap> if true then
> > Print("true\nfail\n\n");
> > else
> > Print("false\n");
> > fi;
> true
> fail
>
>
> # New test case
> gap> Print("Hello\nWorld\n");
> Hello
> World
> """;
true
gap> FileString(Filename(temp_dir, "test_in_014.tst"), \
> """gap> if true then
> true
> fail
>
> > Print("true\nfail\n\n");
> > else
> > Print("false\n");
> > fi;
>
> # New test case
> gap> if true then
> true
> fail
>
> > Print("true\nfail\n\n");
> > else
> > Print("false\n");
> > fi;
> """);;
gap> Test(Filename(temp_dir, "test_in_014.tst"),
> rec(rewriteToFile := Filename(temp_dir, "test_out_014.tst")));;
gap> StringFile(Filename(temp_dir, "test_out_014.tst")) = \
> """gap> if true then
> > Print("true\nfail\n\n");
> > else
> > Print("false\n");
> > fi;
> true
> fail
>
>
> # New test case
> gap> if true then
> > Print("true\nfail\n\n");
> > else
> > Print("false\n");
> > fi;
> true
> fail
>
> """;
true
Loading