Skip to content

test/system: Simplify checking if the container started or not#1367

Merged
debarshiray merged 3 commits intocontainers:mainfrom
debarshiray:wip/rishi/test-system-container_started-simplify
Sep 19, 2023
Merged

test/system: Simplify checking if the container started or not#1367
debarshiray merged 3 commits intocontainers:mainfrom
debarshiray:wip/rishi/test-system-container_started-simplify

Conversation

@debarshiray
Copy link
Copy Markdown
Member

Bats' run helper is not necessary to merely check if a command succeeded or not [1]. In this case, it's idiomatic to pipe the output directly to grep(1) and use it as the condition for an if branch.

[1] https://bats-core.readthedocs.io/en/stable/writing-tests.html

@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/9327b6f5fd3949c892ff29cab2cd51f0

✔️ unit-test SUCCESS in 8m 31s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 27s
✔️ unit-test-restricted SUCCESS in 7m 31s
system-test-fedora-rawhide FAILURE in 33m 46s
✔️ system-test-fedora-38 SUCCESS in 26m 31s
✔️ system-test-fedora-37 SUCCESS in 26m 02s

debarshiray added a commit to debarshiray/toolbox that referenced this pull request Sep 19, 2023
Bats' 'run' helper is not necessary to merely check if a command
succeeded or not [1].  In this case, it's idiomatic to pipe the 'output'
directly to grep(1) and use it as the condition for an 'if' branch.

[1] https://bats-core.readthedocs.io/en/stable/writing-tests.html

containers#1367
debarshiray added a commit to debarshiray/toolbox that referenced this pull request Sep 19, 2023
@debarshiray debarshiray force-pushed the wip/rishi/test-system-container_started-simplify branch from 40aee85 to 40fd406 Compare September 19, 2023 17:03
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/5441261ee8c643f1a6a20afc75aa24a0

✔️ unit-test SUCCESS in 9m 54s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 14s
✔️ unit-test-restricted SUCCESS in 7m 19s
system-test-fedora-rawhide FAILURE in 33m 48s
✔️ system-test-fedora-38 SUCCESS in 26m 37s
✔️ system-test-fedora-37 SUCCESS in 26m 34s

Bats' 'run' helper is not necessary to merely check if a command
succeeded or not [1].  It also complicates using pipes to feed the
output of 'podman logs' into grep(1) [1].

In this case, it's idiomatic to pipe the 'output' directly to grep(1)
and use it as the condition for an 'if' branch.

[1] https://bats-core.readthedocs.io/en/stable/writing-tests.html

containers#1367
debarshiray added a commit to debarshiray/toolbox that referenced this pull request Sep 19, 2023
debarshiray added a commit to debarshiray/toolbox that referenced this pull request Sep 19, 2023
@debarshiray debarshiray force-pushed the wip/rishi/test-system-container_started-simplify branch from 40fd406 to 5bcb375 Compare September 19, 2023 17:48
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/c421793ed2934654a61ac405237bf4b2

✔️ unit-test SUCCESS in 14m 37s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 23s
✔️ unit-test-restricted SUCCESS in 7m 36s
system-test-fedora-rawhide FAILURE in 57m 00s
system-test-fedora-38 TIMED_OUT in 40m 35s
system-test-fedora-37 TIMED_OUT in 40m 35s

@debarshiray debarshiray force-pushed the wip/rishi/test-system-container_started-simplify branch from 5bcb375 to 363c3f8 Compare September 19, 2023 21:41
@softwarefactory-project-zuul
Copy link
Copy Markdown

Build failed.
https://softwarefactory-project.io/zuul/t/local/buildset/76b4fc712e9d4e25a764e7581f6fa93f

✔️ unit-test SUCCESS in 7m 58s
✔️ unit-test-migration-path-for-coreos-toolbox SUCCESS in 3m 10s
✔️ unit-test-restricted SUCCESS in 6m 59s
system-test-fedora-rawhide FAILURE in 31m 49s
✔️ system-test-fedora-38 SUCCESS in 23m 12s
✔️ system-test-fedora-37 SUCCESS in 23m 30s

Copy link
Copy Markdown
Member

@martymichal martymichal left a comment

Choose a reason for hiding this comment

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

Straightforward and nicer to read 👍. But makes me wonder whether this function should be merged with start_container() as it does the same with the difference of also ensuring the container did, in fact, start.

@debarshiray
Copy link
Copy Markdown
Member Author

Straightforward and nicer to read 👍. But makes me wonder whether this function should be merged with start_container() as it does the same with the difference of also ensuring the container did, in fact, start.

Thanks for the review, @HarryMichal ! I have wondered that too. I didn't want to change too many things here, because my main objective here is to be able to enforce shellcheck(1) on all the Bats tests.

@debarshiray
Copy link
Copy Markdown
Member Author

There are still some test failures on Fedora Rawhide. For example:

fedora-rawhide | not ok 3 help: Run command 'help' in 145ms
fedora-rawhide | # (from function `assert_line' in file test/system/libs/bats-assert/src/assert.bash, line 479,
fedora-rawhide | #  in test file test/system/002-help.bats, line 45)
fedora-rawhide | #   `assert_line --index 0 --partial "toolbox(1)"' failed
fedora-rawhide | # /usr/bin/man
fedora-rawhide | #
fedora-rawhide | # -- line does not contain substring --
fedora-rawhide | # index     : 0
fedora-rawhide | # substring : toolbox(1)
fedora-rawhide | # line      : troff:<standard input>:33: warning: cannot select font 'C'
fedora-rawhide | # --

I believe these are because of changes in various other components in Fedora 39, which we need to track down one by one and work out a fix.

In the mean time, I am going to temporarily override these failures.

@debarshiray debarshiray merged commit 363c3f8 into containers:main Sep 19, 2023
@debarshiray debarshiray deleted the wip/rishi/test-system-container_started-simplify branch September 19, 2023 22:51
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.

2 participants