Skip to content

Conversation

@kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Oct 18, 2025

This is the first part of tests/integration cleanup I wanted to do for quite some time.

Mostly some cleanups, a couple of real fixes.

High level overview:

  • tests/int/README: update
  • tests/int: remove useless/obvious comments
  • tests/int/netdev: slight refactoring
  • tests/int: use run with a status check
  • tests/int: remove bogus $status checks
  • tests/int/cgroups.bats: fix a wrong comment
  • tests/int/checkpoint: fix using run twice
  • tests/int/delete: fix pause test for rootless case
  • tests/int/cgroups: use heredoc to break a long line

Please see individual commits for details.

1. Remove the devicemapper driver mentions, and is it no longer
   supported by docker (or podman).

2. Remove the test example -- we have plenty of real ones.

3. Add a link to (well written and extensive) bats documentation.

4. Fix capitalization in a sentence.

Signed-off-by: Kir Kolyshkin <[email protected]>
This is a bit opinionated, but some comments in integration tests do not
really help to understand the nature of the tests being performed by
stating something very obvious, like

	# run busybox detached
	runc run -d busybox

To make things worse, these not-so-helpful messages are being
copy/pasted over and over, and that is the main reason to remove them.

Signed-off-by: Kir Kolyshkin <[email protected]>
Move the repetitive code and comment into setup_netns.

Signed-off-by: Kir Kolyshkin <[email protected]>
...instead of an explicit or absent status check.

Signed-off-by: Kir Kolyshkin <[email protected]>
Commands that are not run via "run" helper (cat, mkdir, __runc)
do not set $status, so it makes no sense to check it.

Fixes: 94505a0, ed54837
Signed-off-by: Kir Kolyshkin <[email protected]>
This misleading comment is obviously a copy/paste from the previous
test. Fix it.

Fixes: dd69623
Signed-off-by: Kir Kolyshkin <[email protected]>
In our bats tests, runc itself is a wrapper which calls bats run helper,
so using "run runc" is wrong as it results in calling run helper twice.

Fixes: 8d180e9
Signed-off-by: Kir Kolyshkin <[email protected]>
The "runc delete --force [paused container]" test case does not check
runc pause exit code, and if added, the test fails in rootless tests,
because:
 - not all rootless tests have access to cgroups;
 - rootless containers doesn't have default cgroups path.

To fix, add:
  - setup for rootless case;
  - require cgroups_freezer;
  - runc pause exit code check.

Signed-off-by: Kir Kolyshkin <[email protected]>
This is mostly to improve readability. While at it, make the script more
robust by adding -e option to shell. The exception is echo $pid which is
opportunistic and may fail depending on the order of pids in the file.

Also, remove the empty comment and a shellcheck annotation.

Fixes: c91fe9a
Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin marked this pull request as draft October 18, 2025 22:41
@kolyshkin kolyshkin marked this pull request as ready for review October 18, 2025 23:52
@kolyshkin kolyshkin requested review from Copilot, lifubang, rata and thaJeztah and removed request for Copilot and rata October 20, 2025 18:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR performs cleanup and refactoring of the integration tests, removing unnecessary comments, fixing minor bugs, and modernizing test patterns. The changes improve code maintainability without altering test functionality.

Key changes:

  • Removed obvious/redundant comments like "run busybox detached" and "check state"
  • Converted old-style run + [ "$status" -eq 0 ] patterns to the more concise run -0 or run ! syntax
  • Fixed rootless pause test requirements and a checkpoint test using run twice
  • Refactored netdev tests to reduce duplication

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/integration/userns.bats Modernized status checks using run ! syntax
tests/integration/update.bats Removed redundant comments and modernized run commands
tests/integration/umask.bats Removed obvious comment
tests/integration/tty.bats Removed redundant comments throughout
tests/integration/state.bats Removed obvious comments
tests/integration/start_hello.bats Removed redundant comments
tests/integration/start_detached.bats Removed obvious comments
tests/integration/start.bats Removed redundant comments
tests/integration/scheduler.bats Modernized run command with status check
tests/integration/pidfd-socket.bats Removed bogus status checks after commands
tests/integration/pause.bats Removed obvious comments
tests/integration/netdev.bats Refactored with setup_netns function and modernized run commands
tests/integration/mask.bats Removed obvious comments
tests/integration/list.bats Removed redundant comment
tests/integration/kill.bats Removed obvious comments
tests/integration/exec.bats Removed redundant comments
tests/integration/events.bats Removed obvious comments
tests/integration/delete.bats Added rootless requirements and fixed pause test
tests/integration/debug.bats Removed obvious comments
tests/integration/create.bats Removed redundant comments
tests/integration/checkpoint.bats Removed obvious comments and fixed duplicate run usage
tests/integration/cgroups.bats Improved readability with heredoc and fixed comment
tests/integration/README.md Updated documentation with better references

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@lifubang lifubang merged commit 3a56046 into opencontainers:main Oct 22, 2025
36 checks passed
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.

3 participants