perf(ci): remove container from unit-tests jobs#19678
Draft
perf(ci): remove container from unit-tests jobs#19678
Conversation
|
Skipping CI for Draft Pull Request. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path=".github/actions/cache-go-dependencies/action.yaml" line_range="99-108" />
<code_context>
run: make deps --always-make
shell: bash
+
+ - name: Stabilize file mtimes for Go test cache
+ run: |
+ # Go's test cache validates inputs by (path, size, mtime) — NOT content.
+ # git checkout sets all mtimes to "now", which differs between CI runs,
+ # causing every test to miss the cache. Setting a fixed mtime makes the
+ # test cache hit across runs when file content hasn't changed.
+ #
+ # Risk: if a non-.go testdata file changes content but not size between
+ # commits, the test cache could return a stale result. This is extremely
+ # rare and Go's own test cache has the same inherent limitation.
+ git ls-files | xargs touch -t 200101010000
+ # Also stabilize directory mtimes — tests using os.ReadDir or
+ # filepath.Walk check directory mtimes too. git ls-files only
</code_context>
<issue_to_address>
**issue (bug_risk):** Use NUL-delimited output from `git ls-files` to handle paths with spaces or special characters safely.
The current `git ls-files | xargs touch -t ...` pipeline will mis-handle paths containing spaces, quotes, or other shell-special characters because `xargs` splits on whitespace. Using NUL-delimited output avoids that:
```bash
git ls-files -z | xargs -0 touch -t 200101010000
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
davdhacs
force-pushed
the
davdhacs/matrix-unittests
branch
from
4c72a6f to
0df8a9d
Compare
Contributor
|
Images are ready for the commit at 11e1696. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19678 +/- ##
===========================================
- Coverage 49.59% 38.23% -11.37%
===========================================
Files 2756 3996 +1240
Lines 208036 269882 +61846
===========================================
+ Hits 103183 103193 +10
- Misses 97192 159015 +61823
- Partials 7661 7674 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
davdhacs
force-pushed
the
davdhacs/matrix-unittests
branch
from
e661d15 to
20afba9
Compare
1 task
Contributor
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe changes refactor CI/CD pipelines and build systems, removing GOMODCACHE from dependency caching, simplifying module preparation steps, and migrating tests from container-based to host-based execution with per-job sharding support. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/scanner-build.yaml:
- Around line 104-107: The job using the composite action "Cache Go
dependencies" currently calls uses: ./.github/actions/cache-go-dependencies
without enabling the module cache; set the composite action input that enables
GOMODCACHE restore by adding a with: cache-mod: 'true' (alongside the existing
env: GOARCH: ${{ matrix.goarch }}), so the action restores the
architecture-independent module cache across matrix entries and avoids
re-downloading modules for each GOARCH.
In @.github/workflows/unit-tests.yaml:
- Around line 252-257: The readiness loops that run the shell snippet "for i in
$(seq 1 30); do docker exec postgres pg_isready -h 127.0.0.1 && break; sleep 1;
done" currently continue even on timeout; change each such loop so after it
finishes you test whether pg_isready succeeded (e.g., check the loop result or
call docker exec pg_isready once more), and if it failed print "docker logs
postgres" to stdout/stderr and exit with non-zero (exit 1) so the workflow step
fails fast on timeout. Update both occurrences of that loop to include the
post-loop failure check, logs dump, and exit.
- Around line 441-447: The job's matrix defines matrix.variant.env but the
"Local roxctl tests" step does not use it; update the "Local roxctl tests" step
to consume the matrix variable by either adding an env: block that sets
NO_BATS_ROXCTL_REBUILD using ${{ matrix.variant.env }} or by inlining the value
when running bats (for example prefix the command with ${{ matrix.variant.env
}}); target the "Local roxctl tests" step in the workflow and ensure it
references matrix.variant.env so the NO_BATS_ROXCTL_REBUILD=1 optimization is
actually applied.
In `@make/protogen.mk`:
- Line 37: SCANNER_DIR is using recursive expansion ("=") which runs the
embedded go mod download during Makefile parse and causes module fetch latency;
change its definition to an immediate/simple assignment using ":=" so the shell
command executes once and the value is cached, preventing repeated parse-time
downloads that also affect SCANNER_PROTO_BASE_PATH, ALL_SCANNER_PROTOS and
SCANNER_M_ARGS downstream references.
In `@Makefile`:
- Around line 560-576: The migrator subtarget is truncating the shared
GO_TEST_OUTPUT_PATH and overwriting the main shard's logs; update the
go-postgres-unit-tests-migrator recipe so its final pipe uses append mode
(change its "tee $(GO_TEST_OUTPUT_PATH)" to "tee -a $(GO_TEST_OUTPUT_PATH)") so
migrator results are appended instead of replacing the main shard output (leave
go-postgres-unit-tests-main behavior as the initial/overwrite writer).
In `@pkg/containers/detection_test.go`:
- Around line 7-10: TestContainerDetection is non-verifying because it only logs
IsRunningInContainer(); refactor the detection logic to be testable and add
deterministic unit tests that assert expected results using controlled
inputs/fixtures. Specifically, make the core detection functions accept
injectable sources (e.g., readers/paths or an interface used by
IsRunningInContainer) or extract parsing helpers (e.g., parseCgroup, parseProc1)
so you can write table-driven tests that call those helpers with fixture data;
then update TestContainerDetection to either assert the smoke behavior against a
mocked source or keep it as a simple runtime smoke test but add new unit tests
(e.g., TestParseCgroup_Docker, TestParseProc1_Systemd) that validate
deterministic outcomes. Ensure references: IsRunningInContainer,
TestContainerDetection, and any new helper names you create are used so
reviewers can locate and verify the changes.
In `@tests/roxctl/bats-tests/helpers.bash`:
- Around line 14-16: The helper yq_multidoc currently masks failures because the
pipeline returns grep's exit code; modify yq_multidoc so the pipeline preserves
yq's exit status (e.g., run the pipeline under pipefail within the function) and
ensure the function exits with yq's error code when yq fails while still
filtering out '---' lines with grep -v '^---$'; update the yq_multidoc function
to use that pattern so callers reliably see yq failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5ba8def7-56ea-4d28-b90b-47cc33faaf6c
📒 Files selected for processing (15)
.github/actions/cache-go-dependencies/action.yaml.github/workflows/build.yaml.github/workflows/scanner-build.yaml.github/workflows/unit-tests.yamlMakefilemake/protogen.mkpkg/containers/detection_test.gopkg/logging/rate_limited_logger.goqa-tests-backend/scripts/copy_scanner_protos.shroxctl/common/zipdownload/download_zip_test.goscripts/ci/lib.shscripts/go-tool.shtests/roxctl/bats-tests/helpers.bashtests/roxctl/bats-tests/local/roxctl-netpol-generate-development.batstests/roxctl/bats-tests/local/roxctl-netpol-generate-release.bats
💤 Files with no reviewable changes (1)
- .github/workflows/build.yaml
pkg/containers/detection_test.go
Outdated
Comment on lines
+7
to
+10
| // Verify container detection runs without error. The result depends on the | ||
| // environment (container vs host runner) and both are valid CI configurations. | ||
| func TestContainerDetection(t *testing.T) { | ||
| if _, ok := os.LookupEnv("GITHUB_ACTIONS"); ok { | ||
| assert.True(t, IsRunningInContainer()) | ||
| } else { | ||
| assert.False(t, IsRunningInContainer()) | ||
| } | ||
| t.Logf("IsRunningInContainer() = %v", IsRunningInContainer()) |
Contributor
There was a problem hiding this comment.
This test is non-verifying and now always passes.
TestContainerDetection currently only logs output, so regressions in IsRunningInContainer() won’t be caught. If CI can run in both host/container modes, keep this as a smoke test only alongside deterministic unit tests that assert behavior via controlled inputs (e.g., injectable detection sources/fixtures), instead of runtime-environment expectations.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/containers/detection_test.go` around lines 7 - 10, TestContainerDetection
is non-verifying because it only logs IsRunningInContainer(); refactor the
detection logic to be testable and add deterministic unit tests that assert
expected results using controlled inputs/fixtures. Specifically, make the core
detection functions accept injectable sources (e.g., readers/paths or an
interface used by IsRunningInContainer) or extract parsing helpers (e.g.,
parseCgroup, parseProc1) so you can write table-driven tests that call those
helpers with fixture data; then update TestContainerDetection to either assert
the smoke behavior against a mocked source or keep it as a simple runtime smoke
test but add new unit tests (e.g., TestParseCgroup_Docker,
TestParseProc1_Systemd) that validate deterministic outcomes. Ensure references:
IsRunningInContainer, TestContainerDetection, and any new helper names you
create are used so reviewers can locate and verify the changes.
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new Go unit-test sharding logic (matrix.shard.packages + grep -Ev exclude-pattern) is fairly regex-heavy and implicit; consider extracting this into a small script or helper with tests so future package moves don’t silently drop or duplicate coverage across shards.
- The
Cache timing debugstep inunit-tests.yamlwill run on every CI execution and may add noise/overhead long-term; you might want to gate it behind a debug flag or remove it once the cache behavior is validated. - The
go-tool.shchange relies onTOOL=testto select fixed ldflags for caching; it may be worth centralizing or validating howTOOLis set for all test entry points to avoid inconsistent behavior between differentgo testinvocations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new Go unit-test sharding logic (matrix.shard.packages + grep -Ev exclude-pattern) is fairly regex-heavy and implicit; consider extracting this into a small script or helper with tests so future package moves don’t silently drop or duplicate coverage across shards.
- The `Cache timing debug` step in `unit-tests.yaml` will run on every CI execution and may add noise/overhead long-term; you might want to gate it behind a debug flag or remove it once the cache behavior is validated.
- The `go-tool.sh` change relies on `TOOL=test` to select fixed ldflags for caching; it may be worth centralizing or validating how `TOOL` is set for all test entry points to avoid inconsistent behavior between different `go test` invocations.
## Individual Comments
### Comment 1
<location path="scripts/ci/lib.sh" line_range="2340-2344" />
<code_context>
- description="${description//>/>}"
+ # XML escape description (\ before & needed for bash 5.2+ where
+ # & in replacement refers to the matched pattern)
+ description="${description//&/\&}"
+ description="${description//\"/\"}"
+ description="${description//\'/\'}"
+ description="${description//</\<}"
+ description="${description//>/\>}"
cat << _EO_CASE_HEADER_ >> "${junit_file}"
</code_context>
<issue_to_address>
**issue (bug_risk):** The added backslashes in the XML-escape replacements will produce invalid escaped output.
In bash parameter expansion the replacement part is literal; `&` is not treated specially. Prefixing it with a backslash causes an actual backslash to appear in the output, e.g. `
&` instead of `&`, which corrupts the XML. The original replacements without backslashes (`"${description//&/&}"`, etc.) are correct in bash 5.2+. If you saw an issue specific to 5.2, please reproduce it in a minimal script rather than changing the escaping logic here.
</issue_to_address>
### Comment 2
<location path="make/protogen.mk" line_range="37" />
<code_context>
GENERATED_API_SWAGGER_SPECS_V2 = $(API_SERVICE_PROTOS_V2:%.proto=$(GENERATED_BASE_PATH)/%.swagger.json)
-SCANNER_DIR = $(shell go list -f '{{.Dir}}' -m github.com/stackrox/scanner)
+SCANNER_DIR := $(shell go mod download github.com/stackrox/scanner >/dev/null 2>&1; go list -f '{{.Dir}}' -m github.com/stackrox/scanner)
ifneq ($(SCANNER_DIR),)
SCANNER_PROTO_BASE_PATH = $(SCANNER_DIR)/proto
</code_context>
<issue_to_address>
**suggestion (performance):** Running `go mod download` unconditionally at variable expansion time may introduce unnecessary network and performance overhead.
Because `SCANNER_DIR` uses `:=` with an inline `go mod download`, this runs at Makefile evaluation for any invocation that includes this file, even when scanner protos aren’t used. That adds startup latency and can cause unexpected network access or offline failures. Instead, move the download into a dedicated target (e.g., as a dependency of the protogen targets) or guard it conditionally (e.g., only run if `go list` fails) so it doesn’t execute on every `make` run.
```suggestion
SCANNER_DIR := $(shell go list -f '{{.Dir}}' -m github.com/stackrox/scanner 2>/dev/null || (go mod download github.com/stackrox/scanner >/dev/null 2>&1 && go list -f '{{.Dir}}' -m github.com/stackrox/scanner))
```
</issue_to_address>
### Comment 3
<location path="roxctl/common/zipdownload/download_zip_test.go" line_range="210-211" />
<code_context>
_, err := os.Stat(path)
- // Expect "no such file or directory" - meaning the file wasn't created
- assert.ErrorIs(t, err, fs.ErrNotExist, "Malicious file should not exist at %s", path)
+ // Expect an error (not exist or permission denied) - meaning the file wasn't created
+ assert.Error(t, err, "Malicious file should not be accessible at %s", path)
}
}
</code_context>
<issue_to_address>
**suggestion (testing):** Relaxing the assertion to any error weakens the guarantee that path traversal is prevented.
The previous `assert.ErrorIs(t, err, fs.ErrNotExist)` ensured we specifically failed when the file didn’t exist. Using only `assert.Error` now would also pass on unrelated errors (e.g. earlier I/O failures), which can mask regressions. Since we want to permit `ErrNotExist` or `ErrPermission`, please assert those explicitly, for example:
```go
require.Error(t, err, "Malicious file should not be accessible at %s", path)
require.True(t, errors.Is(err, fs.ErrNotExist) || errors.Is(err, fs.ErrPermission),
"unexpected error for malicious path %s: %v", path, err)
```
This keeps the test environment-agnostic while still verifying the intended security behavior precisely.
Suggested implementation:
```golang
"bytes"
"errors"
"io/fs"
"os"
"path/filepath"
"strings"
```
```golang
for _, path := range checkPaths {
_, err := os.Stat(path)
// Expect an error (not exist or permission denied) - meaning the file wasn't created
require.Error(t, err, "Malicious file should not be accessible at %s", path)
require.True(t, errors.Is(err, fs.ErrNotExist) || errors.Is(err, fs.ErrPermission),
"unexpected error for malicious path %s: %v", path, err)
}
}
```
1. Ensure `"github.com/stretchr/testify/require"` is imported in this file, similar to the existing `"github.com/stretchr/testify/assert"` import.
2. If `assert` is no longer used elsewhere in this test file, you can safely remove the `assert` import; otherwise, keep it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/unit-tests.yaml (2)
645-654:⚠️ Potential issue | 🟡 MinorMissing new integration jobs in Slack notification
needs.The
go-integrationandgo-operator-integrationjobs are not included in theneedslist. Failures in these new jobs won't trigger Slack notifications.Proposed fix
needs: - go - go-bench + - go-integration + - go-operator-integration - go-postgres - local-roxctl-tests - openshift-ci-unit-tests - sensor-integration-tests - shell-unit-tests - ui - ui-component🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yaml around lines 645 - 654, The Slack notification "needs" list is missing the new integration jobs; update the YAML `needs` array used for Slack notifications to include the job names "go-integration" and "go-operator-integration" so failures in those jobs will trigger notifications; locate the `needs:` block that currently contains items like "go", "go-bench", "go-postgres", etc., and append "go-integration" and "go-operator-integration" to that list.
664-664:⚠️ Potential issue | 🟡 MinorDuplicate reference and missing
ui-componentinmention_author.
needs.go.outputs.new-jirasappears twice. The second occurrence should likely beneeds.ui-component.outputs.new-jirassince that job also has anew-jirasoutput.Proposed fix
- mention_author: ${{ needs.go.outputs.new-jiras || needs.go-postgres.outputs.new-jiras || needs.local-roxctl-tests.outputs.new-jiras || needs.ui.outputs.new-jiras || needs.go.outputs.new-jiras || needs.shell-unit-tests.outputs.new-jiras || needs.sensor-integration-tests.outputs.new-jiras }} + mention_author: ${{ needs.go.outputs.new-jiras || needs.go-postgres.outputs.new-jiras || needs.local-roxctl-tests.outputs.new-jiras || needs.ui.outputs.new-jiras || needs.ui-component.outputs.new-jiras || needs.shell-unit-tests.outputs.new-jiras || needs.sensor-integration-tests.outputs.new-jiras }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yaml at line 664, The mention_author expression contains a duplicated needs.go.outputs.new-jiras and is missing the ui-component job; update the list used for mention_author to replace the second needs.go.outputs.new-jiras with needs.ui-component.outputs.new-jiras so the expression reads ...needs.go.outputs.new-jiras || needs.go-postgres.outputs.new-jiras || needs.local-roxctl-tests.outputs.new-jiras || needs.ui.outputs.new-jiras || needs.ui-component.outputs.new-jiras || needs.shell-unit-tests.outputs.new-jiras || needs.sensor-integration-tests.outputs.new-jiras (referencing the mention_author variable and the jobs needs.go, needs.ui, needs.ui-component, needs.go-postgres, needs.local-roxctl-tests, needs.shell-unit-tests, and needs.sensor-integration-tests).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/unit-tests.yaml:
- Around line 114-144: The go-integration job is missing the GOTOOLCHAIN=auto
override; update the actions/setup-go@v6 step (the step with "uses:
actions/setup-go@v6" inside the go-integration job) to set GOTOOLCHAIN=auto
(e.g., add an env: GOTOOLCHAIN: auto on that step or otherwise export
GOTOOLCHAIN=auto before running setup-go) so the job uses the same toolchain
override as the other Go jobs.
---
Outside diff comments:
In @.github/workflows/unit-tests.yaml:
- Around line 645-654: The Slack notification "needs" list is missing the new
integration jobs; update the YAML `needs` array used for Slack notifications to
include the job names "go-integration" and "go-operator-integration" so failures
in those jobs will trigger notifications; locate the `needs:` block that
currently contains items like "go", "go-bench", "go-postgres", etc., and append
"go-integration" and "go-operator-integration" to that list.
- Line 664: The mention_author expression contains a duplicated
needs.go.outputs.new-jiras and is missing the ui-component job; update the list
used for mention_author to replace the second needs.go.outputs.new-jiras with
needs.ui-component.outputs.new-jiras so the expression reads
...needs.go.outputs.new-jiras || needs.go-postgres.outputs.new-jiras ||
needs.local-roxctl-tests.outputs.new-jiras || needs.ui.outputs.new-jiras ||
needs.ui-component.outputs.new-jiras || needs.shell-unit-tests.outputs.new-jiras
|| needs.sensor-integration-tests.outputs.new-jiras (referencing the
mention_author variable and the jobs needs.go, needs.ui, needs.ui-component,
needs.go-postgres, needs.local-roxctl-tests, needs.shell-unit-tests, and
needs.sensor-integration-tests).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d063e5d8-7b74-4569-ae22-176d3fd899fc
📒 Files selected for processing (2)
.github/workflows/unit-tests.yamlmake/protogen.mk
davdhacs
force-pushed
the
davdhacs/matrix-unittests
branch
3 times, most recently
from
5662695 to
f9919ed
Compare
Tests don't need real version info. Use fixed version strings (0.0.0-test) instead of git-describe values for test ldflags. This makes link ActionIDs stable across commits, enabling Go's test result cache to hit. Also adds -buildvcs=false to stop Go 1.18+'s VCS stamping which independently changes link ActionIDs on every commit. Builds keep the current XDef/status.sh mechanism unchanged. Verified locally: tests cache across commits with fixed ldflags. Combined with the mtime fix (#19395), this enables full warm test caching in CI (2.5-8.2x faster test jobs). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Go's test cache validates inputs by (path, size, mtime) — not content. git checkout sets all file mtimes to the current time, which differs between CI runs, causing test results to never be cached. Fix: set all tracked file mtimes to a fixed date (2001-01-01) after checkout and cache restore. This makes the test cache hit across runs when file content hasn't changed. Measured impact (from PR #19201): - go unit tests: 35m → 8m (4.5x) - go-postgres: 31m → 36s (52x) - sensor-integration: 26m → 2.5m (11x) - Test cache hit rate: 97.6% (681/698 packages) Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
make chart → make tag → git describe fails in shallow clone. Set BUILD_TAG=0.0.0-test to bypass git describe entirely, matching the approach used for test ldflags. Removes the need for fetch-depth: 0 or git fetch --tags. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace cache-mod approach with clean removal from #19688: - Remove GOMODCACHE save/restore and cache-mod input entirely - Remove make deps from action - Remove go mod tidy from build.yaml - Scanner proto: ensure-scanner-protos lazy download in protogen.mk - copy_scanner_protos.sh: targeted go mod download Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
davdhacs
force-pushed
the
davdhacs/matrix-unittests
branch
from
f9919ed to
1f31808
Compare
make -C operator/ test-integration also calls make targets that use git describe (via protogen.mk). Shallow checkout has no tags. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…n test GITHUB_ACTIONS is set on all GHA runners regardless of whether they run in a container. /.dockerenv is the actual signal that IsDocker() checks — it exists inside Docker containers and doesn't exist on host runners. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
davdhacs
force-pushed
the
davdhacs/matrix-unittests
branch
from
a531ce6 to
ca27ddf
Compare
Added in #14312 as a CI sanity check for IsRunningInContainer(). The test is inherently circular — it can only verify container detection by checking the same signals the function checks. Actual regression protection comes from local-roxctl bats tests which exercise the central generate command that depends on this. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
davdhacs
force-pushed
the
davdhacs/matrix-unittests
branch
from
c7cc3a4 to
6d9dd2a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/unit-tests.yaml (1)
124-136:⚠️ Potential issue | 🟡 MinorMissing
GOTOOLCHAIN=autooverride for consistency.The
go-integrationjob usesactions/setup-go@v6but lacks theGOTOOLCHAIN=autooverride that other Go jobs include (go,go-operator-integration,go-postgres,go-bench,local-roxctl-tests). This could cause issues if sub-module tools require newer Go versions.Proposed fix
- uses: actions/setup-go@v6 with: go-version-file: go.mod cache: false + # setup-go sets GOTOOLCHAIN=local; override so sub-module tools + # with newer go directives can auto-download. + - name: Override GOTOOLCHAIN + run: echo "GOTOOLCHAIN=auto" >> "$GITHUB_ENV" + - uses: ./.github/actions/job-preamble🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/unit-tests.yaml around lines 124 - 136, The actions/setup-go@v6 step in the go-integration job is missing the GOTOOLCHAIN=auto override used by other Go jobs; update the job to set the environment variable GOTOOLCHAIN to "auto" (either by adding an env block at the job level or adding env: GOTOOLCHAIN: auto to the actions/setup-go@v6 step) so that tools in submodules use the auto toolchain selection and remain consistent with the go, go-operator-integration, go-postgres, go-bench and local-roxctl-tests jobs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/unit-tests.yaml:
- Around line 72-86: The job never applies the matrix's gotags value, so both
GOTAGS variants run identical tests; fix it by prefixing the test command with
the matrix expansion (matrix.gotags) so release tags are set before running
scripts/go-test.sh — i.e., prepend `${{ matrix.gotags }}` to the line that
currently sets GOTAGS and invokes scripts/go-test.sh (the block that constructs
PACKAGES and runs scripts/go-test.sh -timeout 25m -race -cover ... | tee
test-output/test.log) so the GOTAGS matrix value is honored.
In `@roxctl/common/zipdownload/download_zip_test.go`:
- Around line 212-215: The current assertion in the test allows fs.ErrPermission
for all paths which can mask path-traversal regressions; change the check in the
test that verifies the file does not exist (the assert.True using errors.Is(err,
fs.ErrNotExist) || errors.Is(err, fs.ErrPermission)) so that fs.ErrPermission is
only treated as acceptable for specific well-known safe paths (e.g., parentDir,
grandParentDir, "/tmp", "/etc"); for all other paths require errors.Is(err,
fs.ErrNotExist). Locate the failing assertion in download_zip_test.go and
implement a conditional that checks the path variable against those safe
directories and only ORs in errors.Is(..., fs.ErrPermission) for them, otherwise
assert ErrNotExist.
---
Duplicate comments:
In @.github/workflows/unit-tests.yaml:
- Around line 124-136: The actions/setup-go@v6 step in the go-integration job is
missing the GOTOOLCHAIN=auto override used by other Go jobs; update the job to
set the environment variable GOTOOLCHAIN to "auto" (either by adding an env
block at the job level or adding env: GOTOOLCHAIN: auto to the
actions/setup-go@v6 step) so that tools in submodules use the auto toolchain
selection and remain consistent with the go, go-operator-integration,
go-postgres, go-bench and local-roxctl-tests jobs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1b91001d-803a-4e7c-80e1-b3ee0d876622
📒 Files selected for processing (13)
.github/actions/cache-go-dependencies/action.yaml.github/workflows/build.yaml.github/workflows/scanner-build.yaml.github/workflows/unit-tests.yamlMakefilepkg/containers/detection_test.gopkg/logging/rate_limited_logger.goroxctl/common/zipdownload/download_zip_test.goscripts/ci/lib.shscripts/go-tool.shtests/roxctl/bats-tests/helpers.bashtests/roxctl/bats-tests/local/roxctl-netpol-generate-development.batstests/roxctl/bats-tests/local/roxctl-netpol-generate-release.bats
💤 Files with no reviewable changes (2)
- pkg/containers/detection_test.go
- .github/actions/cache-go-dependencies/action.yaml
✅ Files skipped from review due to trivial changes (3)
- pkg/logging/rate_limited_logger.go
- scripts/ci/lib.sh
- .github/workflows/scanner-build.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/roxctl/bats-tests/local/roxctl-netpol-generate-development.bats
- .github/workflows/build.yaml
- tests/roxctl/bats-tests/helpers.bash
- scripts/go-tool.sh
Comment on lines
72
to
+86
| - name: Go Unit Tests | ||
| run: ${{ matrix.gotags }} make go-unit-tests | ||
| run: | | ||
| mkdir -p bin test-output | ||
| set -o pipefail | ||
| # Build package list, filtering out packages that have their own dedicated jobs | ||
| EXCLUDE='sensor/tests|operator/tests|/scanner/e2etests' | ||
| if [[ -n '${{ matrix.shard.exclude-pattern }}' ]]; then | ||
| EXCLUDE="${EXCLUDE}|${{ matrix.shard.exclude-pattern }}" | ||
| fi | ||
| PACKAGES=$(go list ${{ matrix.shard.packages }} | grep -Ev "$EXCLUDE") | ||
| GOTAGS="${GOTAGS:+$GOTAGS,}test" CGO_ENABLED=1 GOEXPERIMENT=cgocheck2 MUTEX_WATCHDOG_TIMEOUT_SECS=30 \ | ||
| scripts/go-test.sh -timeout 25m -race -cover \ | ||
| -coverprofile test-output/coverage.out -v \ | ||
| $PACKAGES \ | ||
| | tee test-output/test.log |
Contributor
There was a problem hiding this comment.
Missing ${{ matrix.gotags }} usage – release tags won't be applied.
The go job defines gotags: [ 'GOTAGS=""', 'GOTAGS=release' ] in its matrix (line 24), but this value is never used in the test step. Compare to other jobs like go-integration (line 143), go-operator-integration (lines 184, 187), and go-postgres (line 245) which all prefix their commands with ${{ matrix.gotags }}. Without this, both matrix variants will run identical tests without release tags.
Proposed fix
- GOTAGS="${GOTAGS:+$GOTAGS,}test" CGO_ENABLED=1 GOEXPERIMENT=cgocheck2 MUTEX_WATCHDOG_TIMEOUT_SECS=30 \
+ ${{ matrix.gotags }} GOTAGS="${GOTAGS:+$GOTAGS,}test" CGO_ENABLED=1 GOEXPERIMENT=cgocheck2 MUTEX_WATCHDOG_TIMEOUT_SECS=30 \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Go Unit Tests | |
| run: ${{ matrix.gotags }} make go-unit-tests | |
| run: | | |
| mkdir -p bin test-output | |
| set -o pipefail | |
| # Build package list, filtering out packages that have their own dedicated jobs | |
| EXCLUDE='sensor/tests|operator/tests|/scanner/e2etests' | |
| if [[ -n '${{ matrix.shard.exclude-pattern }}' ]]; then | |
| EXCLUDE="${EXCLUDE}|${{ matrix.shard.exclude-pattern }}" | |
| fi | |
| PACKAGES=$(go list ${{ matrix.shard.packages }} | grep -Ev "$EXCLUDE") | |
| GOTAGS="${GOTAGS:+$GOTAGS,}test" CGO_ENABLED=1 GOEXPERIMENT=cgocheck2 MUTEX_WATCHDOG_TIMEOUT_SECS=30 \ | |
| scripts/go-test.sh -timeout 25m -race -cover \ | |
| -coverprofile test-output/coverage.out -v \ | |
| $PACKAGES \ | |
| | tee test-output/test.log | |
| - name: Go Unit Tests | |
| run: | | |
| mkdir -p bin test-output | |
| set -o pipefail | |
| # Build package list, filtering out packages that have their own dedicated jobs | |
| EXCLUDE='sensor/tests|operator/tests|/scanner/e2etests' | |
| if [[ -n '${{ matrix.shard.exclude-pattern }}' ]]; then | |
| EXCLUDE="${EXCLUDE}|${{ matrix.shard.exclude-pattern }}" | |
| fi | |
| PACKAGES=$(go list ${{ matrix.shard.packages }} | grep -Ev "$EXCLUDE") | |
| ${{ matrix.gotags }} GOTAGS="${GOTAGS:+$GOTAGS,}test" CGO_ENABLED=1 GOEXPERIMENT=cgocheck2 MUTEX_WATCHDOG_TIMEOUT_SECS=30 \ | |
| scripts/go-test.sh -timeout 25m -race -cover \ | |
| -coverprofile test-output/coverage.out -v \ | |
| $PACKAGES \ | |
| | tee test-output/test.log |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/unit-tests.yaml around lines 72 - 86, The job never
applies the matrix's gotags value, so both GOTAGS variants run identical tests;
fix it by prefixing the test command with the matrix expansion (matrix.gotags)
so release tags are set before running scripts/go-test.sh — i.e., prepend `${{
matrix.gotags }}` to the line that currently sets GOTAGS and invokes
scripts/go-test.sh (the block that constructs PACKAGES and runs
scripts/go-test.sh -timeout 25m -race -cover ... | tee test-output/test.log) so
the GOTAGS matrix value is honored.
Comment on lines
+212
to
+215
| // File must not exist. On non-root runners, paths under /root/ return | ||
| // ErrPermission instead of ErrNotExist — both confirm the file wasn't written. | ||
| assert.True(t, errors.Is(err, fs.ErrNotExist) || errors.Is(err, fs.ErrPermission), | ||
| "Malicious file should not exist at %s, got: %v", path, err) |
Contributor
There was a problem hiding this comment.
Narrow ErrPermission handling to avoid masking traversal regressions.
At Line 214, allowing fs.ErrPermission for all paths makes this defense-in-depth check too permissive. For paths that are expected to be readable in CI (parentDir, grandParentDir, /tmp, /etc), this can hide a real escape side effect.
Proposed tightening
for _, path := range checkPaths {
_, err := os.Stat(path)
- // File must not exist. On non-root runners, paths under /root/ return
- // ErrPermission instead of ErrNotExist — both confirm the file wasn't written.
- assert.True(t, errors.Is(err, fs.ErrNotExist) || errors.Is(err, fs.ErrPermission),
- "Malicious file should not exist at %s, got: %v", path, err)
+ // `/root` may be non-traversable on non-root runners, so ErrPermission is acceptable there.
+ if strings.HasPrefix(path, "/root/") {
+ assert.True(t, errors.Is(err, fs.ErrNotExist) || errors.Is(err, fs.ErrPermission),
+ "Malicious file should not exist at %s, got: %v", path, err)
+ continue
+ }
+ assert.ErrorIs(t, err, fs.ErrNotExist, "Malicious file should not exist at %s", path)
}As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@roxctl/common/zipdownload/download_zip_test.go` around lines 212 - 215, The
current assertion in the test allows fs.ErrPermission for all paths which can
mask path-traversal regressions; change the check in the test that verifies the
file does not exist (the assert.True using errors.Is(err, fs.ErrNotExist) ||
errors.Is(err, fs.ErrPermission)) so that fs.ErrPermission is only treated as
acceptable for specific well-known safe paths (e.g., parentDir, grandParentDir,
"/tmp", "/etc"); for all other paths require errors.Is(err, fs.ErrNotExist).
Locate the failing assertion in download_zip_test.go and implement a conditional
that checks the path variable against those safe directories and only ORs in
errors.Is(..., fs.ErrPermission) for them, otherwise assert ErrNotExist.
Contributor
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
Fix tests and scripts to work on ubuntu-latest host runners (no container) in addition to CI container environments. - rate_limited_logger.go: add /home/runner/work/ path prefix (container used /__w/) - download_zip_test.go: accept ErrPermission for /root/ paths (non-root runner returns permission denied, not not-found) - detection_test.go: delete circular test - lib.sh: escape & in bash 5.2+ replacements (verified 4.4-5.3) - helpers.bash: add yq_multidoc helper (yq 4.x adds --- separators) - roxctl-netpol-generate-*.bats: use yq_multidoc All changes are backward-compatible with container environments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 tasks
…ttests # Conflicts: # .github/actions/cache-go-dependencies/action.yaml # .github/workflows/unit-tests.yaml
- Quote $(go env ...) subshells (SC2046) - Suppress SC2086 for deliberate word splitting of package lists - Use _ for unused loop variable (SC2034) - Split export and assignment (SC2155) Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove inline comments from shellcheck directives — shellcheck doesn't support the -- comment syntax on disable directives. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ttests # Conflicts: # .github/workflows/unit-tests.yaml
5 tasks
grep -v returns exit code 1 when no lines match (e.g. yq output is only --- separators), which would look like a yq failure. sed always returns 0 regardless of whether lines were deleted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 1, 2026
davdhacs
added a commit
that referenced
this pull request
Apr 1, 2026
Run remaining unit-tests jobs directly on ubuntu-latest: - ui: remove container, add setup-node - ui-component: remove container, use cypress-io/github-action - local-roxctl-tests: remove container, add setup-go - shell-unit-tests: remove container, add bats-core/bats-action - openshift-ci-unit-tests: remove container Depends on #19712 (host-runner test fixes). Split from #19678. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5 tasks
… into davdhacs/matrix-unittests # Conflicts: # tests/roxctl/bats-tests/helpers.bash
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Comprehensive unit-tests workflow optimization: remove all CI containers, shard Go tests, skip GOMODCACHE, cache integration tests, skip benchmarks on PRs, and optimize Cypress.
Master: 33-36m / 188m compute → 3.2m / 54m compute. 10-11x faster, 71% less compute.
Changes
-count=1) + go-operator-integration.go mod tidyin make targets (validated in style workflow).cypress-io/github-action(caches binary),CYPRESS_VIDEO=false, artifacts upload only on failure.Warm run results (run 23728217606)
Key findings
go mod tidyin make deps downloaded all modules even when tests were cached.SKIP_DEPS=1bypasses this.-count=1on integration tests from 2019 was unnecessary — 38/50 packages use mocks.Test fixes for host-runner environment
TestContainerDetection, zip path perms, postgres locale, rate_limited_logger paths, bash 5.2+ XML escaping, GOTOOLCHAIN=auto, bats-action (SHA-pinned), yq v4 separators.
Based on #19618. Depends on #19617, #19395, #19580. Suggested by @janisz.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Partially generated by AI.