Refs #39167 - add tests for job wizard templates#1025
Refs #39167 - add tests for job wizard templates#1025MariaAga wants to merge 1 commit intotheforeman:masterfrom
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughA new integration test file Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/integration/job_wizard_category_js_test.rb (1)
91-94: Consider a more explicit assertion for readability.The current assertion works correctly, but the intent (button should be enabled) could be clearer with a different approach.
Alternative for clarity
assert_selector(:ouia_component_id, 'run-on-selected-hosts-footer', :wait => 30) run_on_hosts = find(:ouia_component_id, 'run-on-selected-hosts-footer') - assert_includes [nil, 'false'], run_on_hosts[:'aria-disabled'] + refute_equal 'true', run_on_hosts[:'aria-disabled'], 'Run on selected hosts button should be enabled' assert_text 'Run on selected hosts'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/job_wizard_category_js_test.rb` around lines 91 - 94, The test's intent is to assert the "Run on selected hosts" button is enabled, but using assert_includes [nil, 'false'] is unclear; update the assertion on run_on_hosts[:'aria-disabled'] (the variable set by find(:ouia_component_id, 'run-on-selected-hosts-footer')) to be explicit — for example, assert_nil run_on_hosts[:'aria-disabled'] or assert_equal 'false', run_on_hosts[:'aria-disabled'] (or use refute run_on_hosts[:'aria-disabled'] to assert falsiness) so the expectation that the button is enabled is immediately clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/integration/job_wizard_category_js_test.rb`:
- Around line 81-85: The assertion message for the ActiveSupport::Notifications
subscriber mismatch: the subscription check uses
payload[:controller].to_s.end_with?('JobInvocationsController') (inside the
block assigned to subscriber) which matches both web and API controllers, but
the assertion text references Api::V2::JobInvocationsController#create; update
the assertion message at the assertion that checks
posted_to_job_invocations_create (around where subscriber is defined and
asserted) so the message accurately reflects the check (e.g., say
"JobInvocationsController#create was posted" or otherwise remove the Api::V2::
namespace) OR tighten the check to match Api::V2::JobInvocationsController
explicitly (change end_with? to == 'Api::V2::JobInvocationsController') so the
assertion message and check align; adjust whichever approach you choose
consistently for that assertion.
---
Nitpick comments:
In `@test/integration/job_wizard_category_js_test.rb`:
- Around line 91-94: The test's intent is to assert the "Run on selected hosts"
button is enabled, but using assert_includes [nil, 'false'] is unclear; update
the assertion on run_on_hosts[:'aria-disabled'] (the variable set by
find(:ouia_component_id, 'run-on-selected-hosts-footer')) to be explicit — for
example, assert_nil run_on_hosts[:'aria-disabled'] or assert_equal 'false',
run_on_hosts[:'aria-disabled'] (or use refute run_on_hosts[:'aria-disabled'] to
assert falsiness) so the expectation that the button is enabled is immediately
clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 668384fb-92f8-437c-b855-d596c86e71fc
📒 Files selected for processing (1)
test/integration/job_wizard_category_js_test.rb
6274fd1 to
6c22b72
Compare
Lukshio
left a comment
There was a problem hiding this comment.
Good coverage improvement overall, but I’d tighten the controller matching before merge.
|
The test looks unstable, some runs ends with infinite retry |
6c22b72 to
c45933b
Compare
|
CI test failures are related |
c45933b to
9cca4c0
Compare
Follow up for #1024
Tests categories when they come from a feature from a url, or when its the default selected category in the wizard
Tests writing was AI assisted.