🐛 Do not use standard ruleset when running tests locally #535
Conversation
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
WalkthroughAdded a flag to the local test runner invocation to disable default rulesets and updated tests to control run-local behavior via an explicit boolean instead of an environment variable. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Runner as pkg/testing/runner.go
participant Engine as Test Execution (local)
Test->>Runner: call defaultRunner.Run(runLocal=true/false)
alt runLocal true
Runner->>Engine: execute with args [..., "--overwrite", "--enable-default-rulesets=false", ...]
else runLocal false
Runner->>Engine: execute with args [..., "--overwrite", ...]
end
Engine-->>Runner: return results
Runner-->>Test: return test outcome
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/testing/runner.go (1)
164-174: Potential panic: Close called on nil file when OpenFile failsIf os.OpenFile fails, logFile is nil and logFile.Close() will panic. Remove the close or guard it.
Apply:
logFile, err := os.OpenFile(filepath.Join(tempDir, "analysis.log"), os.O_CREATE|os.O_APPEND|os.O_RDWR, 0644) if err != nil { err = fmt.Errorf("failed creating a log file - %w", err) opts.Log.Error(err, "failed during execution") results = append(results, Result{ TestsFilePath: testsFile.Path, Error: err}) - logFile.Close() continue }If you prefer being defensive throughout the function, you can instead guard all early-return/continue sites with:
if logFile != nil { _ = logFile.Close() }
🧹 Nitpick comments (3)
pkg/testing/runner.go (3)
316-316: Prefer MkdirAll and handle the errorMkdir will fail if intermediate dirs are missing or if called twice; MkdirAll is safer. Also, check the error to fail fast instead of surfacing a later “failed reading output”.
-os.Mkdir(filepath.Join(dir, "output"), 0755) +if err := os.MkdirAll(filepath.Join(dir, "output"), 0o755); err != nil { + return "", fmt.Errorf("failed to create output directory: %w", err) +}
264-269: Make verification resilient to multiple rulesets in outputEven with default rules disabled here, relying on outputRulesets[0] is brittle (ordering changes or future defaults could reintroduce multi-ruleset outputs). Prefer selecting the ruleset that matches the test’s RuleID instead of assuming index 0.
If helpful, I can draft a helper like selectRuleSetByID([]konveyor.RuleSet, id string) (konveyor.RuleSet, bool) and update tc.Verify(...) accordingly. Please confirm the exact identifier field in konveyor.RuleSet you want to match on (name vs. id) and I’ll propose a concrete diff.
317-326: Verification Complete:--enable-default-rulesetsFlag Name and Default Confirmed
- Confirmed in
cmd/analyze.go(around line 323) that theanalyzecommand registers
--enable-default-rulesetswith a default value oftrue, via
Flags().BoolVar(&analyzeCmd.enableDefaultRulesets, "enable-default-rulesets", true, …).- Verified in
pkg/testing/runner.go(line 325) that including
"--enable-default-rulesets=false"correctly overrides this default in the run-local test path.- Disabling default rulesets here indeed ensures only the test-provided
rules.yamlis applied, aligning local runs with container behavior.Optional refactor recommendation:
- Consider extracting the assembly of
analyzearguments into a helper function (e.g.,buildAnalyzeArgs) so you can unit-test that--enable-default-rulesets=false(and any future flags) are always included for run-local tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/testing/runner.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T15:06:52.056Z
Learnt from: abrugaro
PR: konveyor/kantra#524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
Applied to files:
pkg/testing/runner.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & test from commit
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #535 +/- ##
==========================================
+ Coverage 23.00% 23.70% +0.70%
==========================================
Files 32 32
Lines 4643 4644 +1
==========================================
+ Hits 1068 1101 +33
+ Misses 3450 3415 -35
- Partials 125 128 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Juan Manuel Leflet Estrada <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/testing/runner_test.go (2)
27-33: Add an assertion to catch the original regression (zero incidents in local mode).Right now the test only checks error presence. To ensure “run local” truly disables default rulesets and still yields incidents from the test ruleset, assert the results are non-empty for the local case. This will prevent regressions of issue #525.
Example (to add after the Run call once you capture results):
if tt.runLocal && len(res) == 0 { t.Errorf("expected non-zero incidents in local mode; got 0") }I can push a patch that captures and validates results for both cases if helpful.
8-11: Stale comment references ENV_RUN_LOCAL—update for clarity.The file no longer uses an ENV_RUN_LOCAL constant. Update the comment to reflect the current approach (explicit TestOptions.RunLocal).
Proposed tweak:
-// ENV_RUN_LOCAL enables the runner to run analysis locally instead of a container -// this must be set in CI containers to make sure we are not launching containers +// Local vs. containerized execution is chosen explicitly via TestOptions.RunLocal in tests. +// RUNNER_IMG is only read when running in containerized mode. const RUNNER_IMG = "RUNNER_IMG"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/testing/runner_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (2)
pkg/testing/runner_test.go (2)
18-19: Good refactor: tests no longer depend on RUN_LOCAL env.Adding an explicit runLocal flag in the table removes hidden env coupling, makes the tests deterministic, and aligns with the PR intent.
25-26: Baseline remote-path case is clear.Keeping a non-local test in the table gives a good baseline for parity checks against local runs.
| RunLocal: tt.runLocal, | ||
| ContainerImage: os.Getenv(RUNNER_IMG), | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Capture and use the returned results to strengthen assertions.
You’re discarding results; capture them so the local-mode test can verify non-empty incidents.
Apply this diff within the subtest:
- _, err = r.Run(testFiles, TestOptions{
+ res, err := r.Run(testFiles, TestOptions{
RunLocal: tt.runLocal,
ContainerImage: os.Getenv(RUNNER_IMG),
})Then add (after the error check) a targeted guard:
if tt.runLocal && len(res) == 0 {
t.Errorf("runner.Run() in local mode returned 0 incidents; expected > 0 based on ./examples/ruleset/discovery.test.yaml")
}Optionally, avoid reading RUNNER_IMG when running locally to reduce noise:
img := ""
if !tt.runLocal {
img = os.Getenv(RUNNER_IMG)
}
// pass ContainerImage: imgRun this quick verification to ensure the runner actually injects the flag in local mode and to confirm no lingering ENV_RUN_LOCAL usages remain:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check runner for disabled default rulesets flag:"
rg -nP --context 2 -- 'enable-default-rulesets=false' pkg/testing/runner.go
echo "Ensure no ENV_RUN_LOCAL references remain:"
rg -nP '\bENV_RUN_LOCAL\b' -g '!**/vendor/**' || echo "OK: no references"Length of output: 625
Strengthen local-mode runner test by capturing and verifying returned incidents
The current test invocation discards the results of r.Run, making it impossible to assert that local mode produces incidents. Let’s update the subtest to:
- Capture the return value from
r.Runand verify it’s non-empty whenRunLocalis true. - Remove the stale
ENV_RUN_LOCALcomment inpkg/testing/runner_test.go. - (Optional) Avoid reading
RUNNER_IMGwhen running locally to reduce noise.
Changes to apply in pkg/testing/runner_test.go:
• Around the r.Run call (lines 43–45):
- _, err = r.Run(testFiles, TestOptions{
+ res, err := r.Run(testFiles, TestOptions{
RunLocal: tt.runLocal,
- ContainerImage: os.Getenv(RUNNER_IMG),
+ // Pass empty image in local mode to silence container-related noise
+ ContainerImage: func() string {
+ if tt.runLocal {
+ return ""
+ }
+ return os.Getenv(RUNNER_IMG)
+ }(),
})• Immediately after the error check, add a guard for local mode:
if tt.runLocal && len(res) == 0 {
t.Errorf("runner.Run() in local mode returned 0 incidents; expected > 0 based on ./examples/ruleset/discovery.test.yaml")
}• Remove the outdated comment at line 8 of pkg/testing/runner_test.go that mentions ENV_RUN_LOCAL.
🤖 Prompt for AI Agents
In pkg/testing/runner_test.go around lines 43–45, the test currently calls r.Run
but discards its return value; change the call to capture the result (e.g., res,
err := r.Run(...)) and after the existing error check add a guard that if
tt.runLocal && len(res) == 0 then fail the test with an informative t.Errorf;
also remove the outdated ENV_RUN_LOCAL comment at line 8 of
pkg/testing/runner_test.go; optionally, when tt.runLocal is true, avoid reading
RUNNER_IMG from the environment and skip setting ContainerImage to reduce noise.
3 participants
https://issues.redhat.com/browse/MTA-6008
Fixes #525
Summary by CodeRabbit