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

Improve bci-init tests #139

Merged
merged 1 commit into from
Jun 20, 2022
Merged

Improve bci-init tests #139

merged 1 commit into from
Jun 20, 2022

Conversation

grisu48
Copy link
Contributor

@grisu48 grisu48 commented Jun 10, 2022

Improve the systemd tests for the bci-init container by adding multiple
new tests for boottime, journald, timedatectl, loginctl, hostnamectl.

Related ticket: https://progress.opensuse.org/issues/111054

Copy link
Collaborator

@dcermak dcermak left a comment

Choose a reason for hiding this comment

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

Looks pretty good, I've added a few suggestions.

I would additionally suggest that you use the pytestmark global variable to just skip this module with docker & systemd >= 248. and Please use the auto_container fixture instead of container. There's really no need to repeat the parametrize for every single test function 😉

tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
@grisu48 grisu48 force-pushed the init branch 3 times, most recently from bed04c0 to e998a0c Compare June 13, 2022 12:00
@grisu48 grisu48 changed the title WIP: Improve bci-init tests Improve bci-init tests Jun 13, 2022
@grisu48 grisu48 requested a review from dcermak June 13, 2022 12:02
@grisu48 grisu48 marked this pull request as ready for review June 13, 2022 12:02
@grisu48 grisu48 requested a review from jlausuch June 13, 2022 12:02
@grisu48
Copy link
Contributor Author

grisu48 commented Jun 13, 2022

I don't know why the pipeline is complaining about the code not being black formatted, IMHO it is:

./format.sh --check
All done! ✨ 🍰 ✨
29 files would be left unchanged.

Copy link
Collaborator

@dcermak dcermak left a comment

Choose a reason for hiding this comment

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

Looks good, just a few nitpicks here and there

tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
@dcermak
Copy link
Collaborator

dcermak commented Jun 13, 2022

I don't know why the pipeline is complaining about the code not being black formatted, IMHO it is:

./format.sh --check
All done! ✨ 🍰 ✨
29 files would be left unchanged.

purge your .tox directly and run tox -e format

tests/test_init.py Outdated Show resolved Hide resolved
@grisu48
Copy link
Contributor Author

grisu48 commented Jun 13, 2022

Yay, it doesn't complain about not being black formatted anymore 🥳

@grisu48 grisu48 requested a review from dcermak June 13, 2022 14:33
@jlausuch
Copy link
Contributor

@dcermak
Copy link
Collaborator

dcermak commented Jun 14, 2022

@grisu48 please add tox -e format --check to your pre-push git hooks 😂

Copy link
Collaborator

@dcermak dcermak left a comment

Choose a reason for hiding this comment

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

One assertion doesn't look right to me, otherwise this is ready to be merged 💪

tests/test_init.py Outdated Show resolved Hide resolved
jlausuch
jlausuch previously approved these changes Jun 14, 2022
Copy link
Contributor

@jlausuch jlausuch left a comment

Choose a reason for hiding this comment

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

Good job :)

Copy link
Collaborator

@dcermak dcermak left a comment

Choose a reason for hiding this comment

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

Please run tox -e format

@grisu48
Copy link
Contributor Author

grisu48 commented Jun 14, 2022

Did that while you have been complaining ;-) Should be good now, after the pipeline passes

@dcermak
Copy link
Collaborator

dcermak commented Jun 14, 2022

@jlausuch @grisu48 the last pipeline (https://github.com/SUSE/BCI-tests/runs/6875884458?check_suite_focus=true) failed with:

E       AssertionError: Unexpected exit code 1 for CommandResult(command=b'systemd-analyze time', exit_status=1, stdout=None, stderr=b"\x1b[0;1;31mBootup is not yet finished (org.freedesktop.systemd1.Manager.FinishTimestampMonotonic=0).\x1b[0m\n\x1b[0;1;31mPlease try again later.\x1b[0m\n\x1b[0;1;31mHint: Use 'systemctl list-jobs' to see active jobs\x1b[0m\n")

This could result in a flaky test, but maybe we should add a HEALTHCHECK to init instead. WDYT?

@grisu48
Copy link
Contributor Author

grisu48 commented Jun 14, 2022

I will have a look at this. Let me have another look, I would like to not push tests with flanky results. Marking as draft until resolved.

@grisu48 grisu48 marked this pull request as draft June 14, 2022 08:32
dcermak added a commit to SUSE/BCI-dockerfile-generator that referenced this pull request Jun 14, 2022
This should prevent issues like
SUSE/BCI-tests#139 (comment) and ensure
that the container is not up before reaching multi-user.target
dcermak added a commit to SUSE/BCI-dockerfile-generator that referenced this pull request Jun 14, 2022
This should prevent issues like
SUSE/BCI-tests#139 (comment) and ensure
that the container is not up before reaching multi-user.target
@grisu48 grisu48 force-pushed the init branch 2 times, most recently from 37e8e9a to 85057b3 Compare June 20, 2022 07:43
@grisu48
Copy link
Contributor Author

grisu48 commented Jun 20, 2022

SUSE/BCI-dockerfile-generator#9 should indeed prevent race conditions where the system target has not yet been reached. Thanks for the improvement.

I've pushed the black formatted test run and think we can proceed with this PR.

@grisu48 grisu48 marked this pull request as ready for review June 20, 2022 07:44
Copy link
Collaborator

@dcermak dcermak left a comment

Choose a reason for hiding this comment

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

Could it be that you force pushed some of your improvements away? There's a bunch of print statements back in there

tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
@grisu48
Copy link
Contributor Author

grisu48 commented Jun 20, 2022

Could it be that you force pushed some of your improvements away? There's a bunch of print statements back in there

Oh damn, the rebase did something weird. Part of the latest changes got removed. Let me check, something went horribly wrong here.

Happy Monday btw ☕

@grisu48 grisu48 force-pushed the init branch 2 times, most recently from cc539b5 to 507e8d2 Compare June 20, 2022 08:37
@grisu48
Copy link
Contributor Author

grisu48 commented Jun 20, 2022

Fixed. Please review again and sorry for the fuzz @dcermak

Copy link
Collaborator

@dcermak dcermak left a comment

Choose a reason for hiding this comment

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

You have to use Dict and not dict.

tests/test_init.py Outdated Show resolved Hide resolved
tests/test_init.py Outdated Show resolved Hide resolved
Improve the systemd tests for the bci-init container by adding multiple
new tests for boottime, journald, timedatectl, loginctl, hostnamectl.
Copy link
Collaborator

@dcermak dcermak left a comment

Choose a reason for hiding this comment

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

Thank you!

@dcermak dcermak merged commit 6afece5 into SUSE:main Jun 20, 2022
@grisu48 grisu48 deleted the init branch June 20, 2022 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants