| Message ID | 20230111233242.16870-1-rybak.a.v@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fixes for commented out code in tests (was "Re: [PATCH] *: fix typos which duplicate a word") | expand |
On 12.01.23 00:32, Andrei Rybak wrote: > [...] > > Here's a patch series that fixes some of the commented out test code. > > I skipped changing the following: > > 1. a minute-long test_expect_failure is commented out in t0014-alias.sh . > Technically, this could be uncommented and marked with `EXPENSIVE` > prerequisite, but it doesn't seem worth it for a `test_expect_failure`. > [ cc Tim Schumacher, who added this test in fef5f7fc43 (t0014: introduce an > alias testing suite, 2018-09-16) ] The reason why this particular test is commented out (and why it mentions a run time of one minute) is because support for detecting external alias loops isn't yet implemented. This means that running that test would spin the test runner until the test times out due to an intentional infinite loop. As soon as that is implemented properly, git would ideally detect the loop after a few iterations at latest, so the test wouldn't require to be marked as 'EXPENSIVE' in the first place. For the context of your patches, skipping adjusting this test is most likely fine, as it references currently unimplemented behavior and it presumably would require more adjustments anyways before finally being enabled. > > [...] > Tim
On Wed, Jan 11, 2023 at 4:05 PM Andrei Rybak <rybak.a.v@gmail.com> wrote: > > [ I apologize for some people getting this twice -- I messed up when > invoking `git send-email` ] > > On 2023-01-06T19:25, Eric Sunshine wrote: > > Not related to your patch at all, but I notice in this test that the > > call to test_when_finished() is commented out: > > > > # test_when_finished "stop_daemon_delete_repo test_insensitive" && > > > > which makes me wonder if it was commented out while the test was being > > debugged but then forgotten, and that the script is now potentially > > leaking a running daemon if something in the test fails after the > > daemon was started, or if the daemon does not shut down on its own as > > it's supposed to do. [cc:+Jeff Hostetler] > > Here's a patch series that fixes some of the commented out test code. Patch 2 is obviously correct. Patches 1 & 3 make sense to me, but it would be nice to have someone familiar with fsmonitor look at #3. As to your notes about other related testcases... > I skipped changing the following: [...] > 2. In t6426-merge-skip-unneeded-updates.sh, second part of the test '2c: Modify > b & add c VS rename b->c' is commented out with an explicit "# FIXME: > rename/add conflicts are horribly broken right now;" above the commented out > part. > [ cc Elijah Newren, author of c04ba51739 (t6046: testcases checking whether > updates can be skipped in a merge, 2018-04-19) ] [...] You missed the cc...but I looked up this email since you did cc me for patch 2/3. Yeah, the commented out code was never tested, because the only thing I could have tested at the time was incorrect results. So I just took a guess at what the improved testing would look like and apparently made 3 small errors in doing so. I have fixed it locally and can submit a patch.
On Wed, Jan 11, 2023 at 5:54 PM Elijah Newren <newren@gmail.com> wrote: > > On Wed, Jan 11, 2023 at 4:05 PM Andrei Rybak <rybak.a.v@gmail.com> wrote: > > > > [ I apologize for some people getting this twice -- I messed up when > > invoking `git send-email` ] > > > > On 2023-01-06T19:25, Eric Sunshine wrote: > > > Not related to your patch at all, but I notice in this test that the > > > call to test_when_finished() is commented out: > > > > > > # test_when_finished "stop_daemon_delete_repo test_insensitive" && > > > > > > which makes me wonder if it was commented out while the test was being > > > debugged but then forgotten, and that the script is now potentially > > > leaking a running daemon if something in the test fails after the > > > daemon was started, or if the daemon does not shut down on its own as > > > it's supposed to do. [cc:+Jeff Hostetler] > > > > Here's a patch series that fixes some of the commented out test code. > > Patch 2 is obviously correct. Patches 1 & 3 make sense to me, but it > would be nice to have someone familiar with fsmonitor look at #3. > > As to your notes about other related testcases... > > > I skipped changing the following: > [...] > > 2. In t6426-merge-skip-unneeded-updates.sh, second part of the test '2c: Modify > > b & add c VS rename b->c' is commented out with an explicit "# FIXME: > > rename/add conflicts are horribly broken right now;" above the commented out > > part. > > [ cc Elijah Newren, author of c04ba51739 (t6046: testcases checking whether > > updates can be skipped in a merge, 2018-04-19) ] > [...] > > You missed the cc...but I looked up this email since you did cc me for > patch 2/3. > > Yeah, the commented out code was never tested, because the only thing > I could have tested at the time was incorrect results. So I just took > a guess at what the improved testing would look like and apparently > made 3 small errors in doing so. I have fixed it locally and can > submit a patch. Submitted here: https://lore.kernel.org/git/pull.1462.git.1673584084761.gitgitgadget@gmail.com/