On 07/07/2025 15:25, redoste wrote:
On Mon Jul 7, 2025 at 11:35 CEST, Phillip Wood wrote:
On 07/07/2025 10:02, Patrick Steinhardt wrote:
I think this exported environment variable now leaks into subsequent
tests, doesn't it? We may want to do it in a subshell.
That's a very good point.
Sure thing, I took inspiration from the previous ssh-agent test and it
exports the ssh-agent variables without spawning a subshell, so I
figured it was fine, but it was only because test_when_finished will
fail in a subshell.
I guess it's probably fine to leak only the ssh-agent variables
accross tests and keep TMPDIR in a subshell.
Hopefully having the ssh-agent variables set after the agent has been
killed doesn't affect the later tests
The idiomatic way to test for an empty file is "test_file_is_empty
<file>" which avoids spawning wc.
Thanks, I will update this.
As this test is an abridged version of the other test that uses
ssh-agent I think it would be more efficient to tweak that test to check
there are no temporary files left over. Our test suite is slow and
tweaking an existing test to improve our coverage instead of adding a
new test means we don't make it any slower than necessary.
I moved this test out of the other ssh-agent one according to the advice
from brian on the previous version. I agree that spawning multiple
ssh-agent is definitly not the best for speed, but is it worth it to
make the test failure less clear?
test_file_is_empty will print a diagnostic message if it fails so it
should be clear what has caused the test failure
I feel like it's not the best to keep checks for temporary files clean
up in a test that (initially) only claims to sign commits.
You can always tweak the test title if you want. The way I see it is
that the changes that are being tested are related to commit signing as
the invariant that we want to assert is that temporary files are cleaned
up after signing commits.
Thanks
Phillip
