test/system: use a random port for the local Docker registry#1822
test/system: use a random port for the local Docker registry#1822Rolv-Apneseth wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the BATS helper script to dynamically assign and persist the Docker registry URI using a randomly assigned host port, rather than using a hardcoded port. This prevents port conflicts during parallel test execution. The review feedback points out that using run followed by assert_success inside _setup_docker_registry is a BATS anti-pattern and can lead to undefined behavior, suggesting executing the commands directly instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| run mkdir -p "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}" | ||
| assert_success | ||
| run cp "${DOCKER_REG_CERTS_DIR}"/domain.crt "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}"/domain.crt | ||
| assert_success |
There was a problem hiding this comment.
According to the repository's general rules, using run followed immediately by assert_success is an anti-pattern in BATS. Additionally, run should not be used inside setup_suite() or functions called by it (such as _setup_docker_registry()), because BATS variables like BATS_TEST_TMPDIR are not defined in that context, which can lead to undefined behavior. These commands should be executed directly instead.
mkdir -p "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}"
cp "${DOCKER_REG_CERTS_DIR}"/domain.crt "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}"/domain.crt
References
- In BATS (Bash Automated Testing System), avoid using 'run' followed immediately by 'assert_success' as it is considered an anti-pattern. Additionally, avoid using 'run' inside 'setup_suite()' or functions called by it, because BATS variables like 'BATS_TEST_TMPDIR' are not defined in that context, which can lead to undefined behavior.
There was a problem hiding this comment.
That's pre-existing code that I just moved, but that makes sense, I could make that change
There was a problem hiding this comment.
Yes, @Rolv-Apneseth is right. These uses of run already existed in the code. I took the liberty to remove some of these in #1825
I couldn't remove all of them because I was running out of time testing all the error paths. Feel free to submit a pull request removing the rest. :)
Hard-coding the port to 50000 can cause intermittent failures when the port happens to already be in use, and also prevents running the test suite more than once at the same time. Let Podman pick a random available port and persist it to a file so test processes in the same run can find it. Signed-off-by: Rolv Apneseth <rolv.apneseth@gmail.com>
82653ee to
d051389
Compare
|
Rebased after #1824 |
debarshiray
left a comment
debarshiray
left a comment
There was a problem hiding this comment.
Thanks for working on this, @Rolv-Apneseth ! I had never used podman port before - cool. :)
| # test processes will still use the same URI for the same test suite run. The | ||
| # contents of the file are set when setting up the local Docker registry. | ||
| readonly DOCKER_REG_URI_FILE="$BATS_SUITE_TMPDIR/docker-reg-uri" | ||
| DOCKER_REG_URI="$(cat "$DOCKER_REG_URI_FILE" 2>/dev/null || echo "")" |
There was a problem hiding this comment.
One quick question: is the || echo "" really needed?
There was a problem hiding this comment.
I thought so yeah, we want the error to be ignored (when the file hasn't been created yet). Maybe there's a better way?
2 participants
See #1817 (comment). I believe this should do the trick @debarshiray