Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI test fix: wait for systemd to come up #3198

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

apostasie
Copy link
Contributor

@apostasie apostasie commented Jul 6, 2024

Seen many times with canary #3189 - see #3186 for full list of pain.

@apostasie
Copy link
Contributor Author

Windows failure is unrelated and likely fixed by #3133

PTAL at your convenience.

@apostasie
Copy link
Contributor Author

Latest push does increase the timeout to 10 seconds.

@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone Jul 8, 2024
@AkihiroSuda AkihiroSuda added the area/ci e.g., CI failure label Jul 8, 2024
@apostasie
Copy link
Contributor Author

@AkihiroSuda changing test condition did change the timing slightly, then changing systemd response (No jobs running. vs jobs listed).

Latest push adapts to that......

This test is not really good honestly...

exit 1
}
done
systemctl list-jobs`).AssertOutContains("jobs")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use AssertOutWithFunc to check stdout == "No jobs running." || stdout == "jobs listed"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see the point in testing stdout at all here.

We should not be in the business of testing systemd, especially on matching random mundane non-error messages.
What matters here is that systemd is up and running (exit code 0), and that we have a test that does not fail randomly (that is the point of this PR).

I would rather leave this stuff as-is for now - until we come back here to write more useful versions of these tests.

What do you think?

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@AkihiroSuda AkihiroSuda merged commit cce9912 into containerd:main Jul 9, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci e.g., CI failure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants