Michael J Gruber [off-list ref] writes:
So, should I prepare a series like:
1: test case and mark known fail
2: the 1 line fix
3: mark test pass
Or should 2+3 be squashed into one?
If "git mv" already has its own sets of tests with a good coverage, please
strive to add a case that covers your fix to an existing script. Then
step #1 above would be a patch to add a few "test_expect_failure" tests to
an existing file, and step #3 would be a patch to flip expect_failure to
expect_success.
And in such a case, for a single liner, all three can be squashed in to a
single patch. It would show what changed in the code and have a few new
test_expect_success tests added to the test suite, and it would be obvious
to anybody who looks at such a change 6 months down the road that the test
cases added by the patch are the cases that did not work without the
changes to the code. It never makes sense to separate steps #2 and #3 for
any fix.
If "git mv" did not have adequate test coverage, then please add a test
script with both expect_success (for cases that should have been there
when somebody worked on "git mv" originally), and expect_failure to expose
the bug you found in your first patch. Again, the second patch would
update the code and flip expect_failure to expect_success.
I see there is t7001-mv and even though it claims to concentrate on its
operation from subdirectory, it has tests for more basic modes of the
operation.
So my recommendation would be to have a single patch that:
(1) retitles t7001;
(2) adds your new -k tests to it; and
(3) adds the 1-liner fix.
Junio C Hamano venit, vidit, dixit 14.01.2009 20:02:
...
If "git mv" did not have adequate test coverage, then please add a test
script with both expect_success (for cases that should have been there
when somebody worked on "git mv" originally), and expect_failure to expose
the bug you found in your first patch. Again, the second patch would
update the code and flip expect_failure to expect_success.
I see there is t7001-mv and even though it claims to concentrate on its
operation from subdirectory, it has tests for more basic modes of the
operation.
So my recommendation would be to have a single patch that:
(1) retitles t7001;
(2) adds your new -k tests to it; and
(3) adds the 1-liner fix.
Sorry to bother you again, even after your detailed reply, but I'm a bit
confused in multiple ways (I guess that makes for multiple bits...).
First, you replied to my post, not my patch v2, but (time-wise) after my
patch v2, so I'm not sure whether your reply applies to v2 as well or
that one is OK.
Second, I took the title of t7001 to mean "mv into subdir" or "mv in and
out subdir" in order to distiguish it from "mv oldname newname", albeit
in English that leaves room from improvement.
Third, various parts of that test are in vastly different styles, and I
haven't found any test writing directions other than "follow what is
there", which leaves several alternatives. (Some use the test-lib repo,
some create their own underneath, some make assumptions about the
contents of "$TEST_DIRECTORY"/../.)
Fourth, t7001-mv is missing any test for "mv -k", and 2 of my 3
additional tests cover cases which work (pass) without the fix, which is
why I kept it separate, being unrelated. Following all your arguments
lead to the conclusion I should squash 2+3 (fix + mark expect_pass) -
until I read your conclusion, which says squash all.
I'm happy to follow any variant ("1+2+3", "1 2+3", "1 2 3", in
increasing order of preference) so there's no need to discuss or explain
this further, just tell me "do x" ;)
Cheers,
Michael