close
Skip to content

Fix CertificateFixture error if previous test fails#10749

Merged
yhabteab merged 2 commits intomasterfrom
fix-certificate-fixture-broken-after-failed-test
Mar 12, 2026
Merged

Fix CertificateFixture error if previous test fails#10749
yhabteab merged 2 commits intomasterfrom
fix-certificate-fixture-broken-after-failed-test

Conversation

@jschmidt-icinga
Copy link
Copy Markdown
Contributor

@jschmidt-icinga jschmidt-icinga commented Mar 9, 2026

This fixes an issue where a when a test-case using the CertificateFixture fails, it causes the next test using this fixture to also fail.

Problem

Originally, CertificateFixture used a symlink to a persistent directory containing the certificates generated over the course of a test run. Then it was changed to move the directory from the persistent store to the working directory and back, to fix testing on Windows without admin accounts. This is fine when the tests all pass, but if one fails, it doesn't move the directory back, which not only forces the tests to regenerate all certificates, but it also fails the test next in line, because it tries to remove the remainder directory with boost::filesystem::remove instead of boost::filesystem::remove_all.

Though no test runs failed because of this that didn't fail anyway, it caused initial confusion what is actually going wrong.

Solution

The certificates are now copied back and forth instead of moving the directory and the cleanup step now deletes recursively instead of relying on the directory being empty. While this potentially causes slightly more disk activity than moving or symlinking, it shouldn't be significant compared to the overall build.

Additional Changes

This PR also adds a new CTest fixture to enforce ordering between the tlsutility tests and the other tests that use TLS certificates. This was necessary with the now fixed removal of the temporary directories in between each test.

This also solves a potential problem where if the tests are reordered the tlsutility tests leave broken certificates and CA behind. The new fixture forces the tests to run into an order where first the tlsutility tests run, then cleanup happens and then the other tests that aren't breaking certificates will run.

@cla-bot cla-bot Bot added the cla/signed label Mar 9, 2026
@jschmidt-icinga jschmidt-icinga added area/tests Unit and environment tests area/api REST API core/quality Improve code, libraries, algorithms, inline docs and removed cla/signed labels Mar 9, 2026
@jschmidt-icinga jschmidt-icinga marked this pull request as draft March 9, 2026 14:26
@jschmidt-icinga jschmidt-icinga force-pushed the fix-certificate-fixture-broken-after-failed-test branch from b844640 to 7128673 Compare March 10, 2026 15:04
@cla-bot cla-bot Bot added the cla/signed label Mar 10, 2026
@jschmidt-icinga jschmidt-icinga force-pushed the fix-certificate-fixture-broken-after-failed-test branch from 7128673 to 25816e4 Compare March 10, 2026 16:02
@jschmidt-icinga jschmidt-icinga added this to the 2.16.0 milestone Mar 10, 2026
@jschmidt-icinga jschmidt-icinga marked this pull request as ready for review March 11, 2026 09:07
Comment thread test/remote-certificate-fixture.hpp
Comment thread test/remote-certificate-fixture.hpp Outdated
Comment thread test/remote-certificate-fixture.hpp Outdated
Comment thread test/remote-certificate-fixture.cpp
This doesn't fix any concrete errors in master, but if tests ever get
reordered for some reason and the tlsutility tests run before the
http tests, they will leave broken certificates behind. This could
be solved by cleaning up manually in the tests, but that again could
cause issues if the tlsutility tests ever run in the middle of the
http ones.

The proper solution is using a CTest fixture to establish a dependency
where the tlsutility tests always run before the tests using a new
CTest fixture that provides the cleanup between these test groups.
@jschmidt-icinga jschmidt-icinga force-pushed the fix-certificate-fixture-broken-after-failed-test branch from 25816e4 to b72ad49 Compare March 11, 2026 13:27
yhabteab
yhabteab previously approved these changes Mar 11, 2026
@jschmidt-icinga jschmidt-icinga force-pushed the fix-certificate-fixture-broken-after-failed-test branch from b72ad49 to e153e6d Compare March 12, 2026 08:35
@yhabteab yhabteab enabled auto-merge March 12, 2026 08:41
@yhabteab yhabteab merged commit 2d172fb into master Mar 12, 2026
28 checks passed
@yhabteab yhabteab deleted the fix-certificate-fixture-broken-after-failed-test branch March 12, 2026 11:28
@yhabteab yhabteab mentioned this pull request Apr 1, 2026
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/api REST API area/tests Unit and environment tests cla/signed core/quality Improve code, libraries, algorithms, inline docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants