Re: [PATCH 18/23] fsmonitor--daemon:: introduce client delay for testing

From: Derrick Stolee <stolee@gmail.com>
Date: Apr 27, 2021, 13:36 UTC
Message-ID: <e09104e9-a75c-e7ec-09cf-18dae9b56a23@gmail.com>
In-Reply-To: <c6d5f045fb5644306a3676e5fa4145ba4c6e9b93.1617291666.git.gitgitgadget@gmail.com>

On 4/1/2021 11:41 AM, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Define GIT_TEST_FSMONITOR_CLIENT_DELAY as a millisecond delay.

This is a second delay introduced in this feature, but the units are different. Could we put a unit in the name? Perhaps a "_MS" suffix.

Show 7 quoted lines

> Introduce an artificial delay when processing client requests.
> This make the CI/PR test suite a little more stable and avoids
> the need to load up test scripts with sleep statements to avoid
> racy failures.  This was mostly seen on 1 or 2 core CI build
> machines where the test script would create a file and quickly
> try to confirm that the daemon had seen it *before* the daemon
> had received the kernel event and causing a test failure.

Isn't the cookie file supposed to prevent this from happening?

Yes, our test suite interacts with the filesystem and Git commands more quickly than a human user would, but Git is used all the time by scripts or build machines to quickly process data. The FS Monitor feature should be robust to such a situation.

I feel that as currently described, this patch is only hiding a bug that shows up during heavy use.

Perhaps the test failures are limited to a small number of specific tests that are checking the FS Monitor daemon in a non-standard way, especially in a way that circumvents the cookie file. In this case, I'd like to see _in this patch_ how the environment variable is used in the test suite.

I understand that it is difficult to simultaneously build a new feature like this in small increments, but the biggest issue I have with the series' organization so far is that we are 18 patches deep and I still haven't seen a single test. This is a case where I think this only serves the purpose of the test suite, so it would be good to delay until only seeing its value in a test script.

Looking ahead, I see that you insert it as a blanket statement in the t7527 test script, which seems like it has potential to hide bugs instead of being an isolated cover for a specific interaction.

As for the code, it all looks correct. However, please update t/README with a description of the new GIT_TEST_* variable.

Thanks, -Stolee