tests: run the reset.sh helper and check test invariants while the test is restored#10033
Conversation
|
With this change when the invariant check fails we know the problem is in the current test and it is not related to previous executions. |
stolowski
left a comment
stolowski
left a comment
There was a problem hiding this comment.
I'm slightly confused (but maybe I don't understand the full picture); this change only moves the reset logic around (from prepare to restore - which I think makes sense), but other than that the call to tests.invariant check remains in prepare - so it's not going to help us narrow problems to current test, it can still be any other test that ran before and didn't reset properly? Am I missing something? Should invariant check be done at the end of reset too?
|
@stolowski thanks for the review, I did the change yesterday but forgot to push that. |
| if [[ "$variant" = full ]]; then | ||
| "$TESTSTOOLS"/cleanup-state pre-invariant | ||
| fi | ||
| tests.invariant check |
There was a problem hiding this comment.
Would it make sense to keep this check too (in case prepare does something wrong) or is it useless after these changes?
There was a problem hiding this comment.
done, sorry, forgot to push that part
There was a problem hiding this comment.
@sergiocazzolato are you sure you pushed the change, I still don't see tests.invariant check being run in the prepare_suite_each function in this file
There was a problem hiding this comment.
I agree here. I think it's ok to move the cleanup to restore, but keep the invariant check both here and in restore. The check does not look too expensive compared to actual tests, and by keeping it in both places we make sure that we start with a good system, and nothing slips through the cracks between restore and prepare.
There was a problem hiding this comment.
Thanks for the review, so now the invariant check is done as part of the prepare and restore of each test. It is a quick check so it should not affect on performance.
This is removed beacuse the bug systemd/systemd#11502 is already closed. Also de degraded test was failing because of this: Error: 2021-03-12 11:28:01 Error executing google:debian-sid-64:tests/main/degraded (mar121119-891844) : ----- + . /home/gopath/src/github.com/snapcore/snapd/tests/lib/systemd.sh + wait_for_service multi-user.target + local service_name=multi-user.target + local state=active ++ seq 300 + for i in $(seq 300) + grep -q ActiveState=active + systemctl show -p ActiveState multi-user.target + return + case "$SPREAD_SYSTEM" in + grep 'State: [d]egraded' + systemctl status State: degraded + echo 'systemctl reports the system is in degraded mode' systemctl reports the system is in degraded mode + systemctl --failed UNIT LOAD ACTIVE SUB DESCRIPTION ● systemd-journal-flush.service loaded failed failed Flush Journal to Persistent Storage
sergiocazzolato
requested review from
anonymouse64,
bboozzoo,
mvo5 and
stolowski
New the invariante clean up and check is done as part of the prepare and restore of each test
anonymouse64
left a comment
anonymouse64
left a comment
There was a problem hiding this comment.
lgtm, let's just squash merge this with a clear commit message
4 participants
The idea of this change is to validate all the tests restore the environment during the restore phase.
So each test should start in a clean environment, then prepare, execute and restore the test, and then clean up the environment and check the invariants, so the next test can run in a clean environment as well.