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

[reiniscirpons]

This PR fixes issue #5685.

Text for release notes

see title

Further details

The implemented fix modifies each test case so that test case input lines come before test case output lines when rewriteToFile is set. This has the side effect of modifying tests on which Test returns true. For example if we let example.tst consist of

# Case 1 
gap> if true then
> Print("true\n");
> else
> Print("false\n");
> fi;
true

# Case 2
gap> if true then
> Print("true\n");
true
> else
> Print("false\n");
> fi;

# Case 3
gap> if true then
true
> Print("true\n");
> else
> Print("false\n");
> fi;

# Case 4
gap> if true then
> Print("true\n");
> else
> Print("false\n");
true
> fi;

# Case 5
gap> if true then
> Print("true\nfail\n");
true
fail
> else
> Print("false\n");
> fi;

# Case 6
gap> if true then
true
fail
> Print("true\nfail\n");
> else
> Print("false\n");
> fi;

# Case 7
gap> if true then
true
fail
foo
bar
baz
> Print("true\nfail\nfoo\nbar\nbaz\n");
> else
> Print("false\n");
> fi;

Running Test("./example.tst"); returns true. Running Test("./example.tst", rec(rewriteToFile := "./example-good.tst")); produces a file with contents

# Case 1 
gap> if true then
> Print("true\n");
> else
> Print("false\n");
> fi;
true

# Case 2
gap> if true then
> Print("true\n");
> else
> Print("false\n");
> fi;
true

# Case 3
gap> if true then
> Print("true\n");
> else
> Print("false\n");
> fi;
true

# Case 4
gap> if true then
> Print("true\n");
> else
> Print("false\n");
> fi;
true

# Case 5
gap> if true then
> Print("true\nfail\n");
> else
> Print("false\n");
> fi;
true
fail

# Case 6
gap> if true then
> Print("true\nfail\n");
> else
> Print("false\n");
> fi;
true
fail

# Case 7
gap> if true then
> Print("true\nfail\nfoo\nbar\nbaz\n");
> else
> Print("false\n");
> fi;
true
fail
foo
bar
baz

[james-d-mitchell]

Looks good to me!

[ChrisJefferson]

Looks good to me too. The test is a bit mind-bending to read, but I agree it correctly tests your fix!

[fingolfin]

Thank you, much appreciated

[fingolfin]

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');

[reiniscirpons]

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.