Skip to content

tests: Fix Compliance distro selector#4445

Merged
tkoscieln merged 1 commit into
osbuild:mainfrom
tkoscieln:fix_compliance_distro_select
May 19, 2026
Merged

tests: Fix Compliance distro selector#4445
tkoscieln merged 1 commit into
osbuild:mainfrom
tkoscieln:fix_compliance_distro_select

Conversation

@tkoscieln
Copy link
Copy Markdown
Collaborator

SSIA

@tkoscieln tkoscieln added 🍼 simple Simple and quick to review 🎭 Playwright Change in Playwright tests labels May 18, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.66%. Comparing base (f591572) to head (37e6633).
⚠️ Report is 1 commits behind head on main.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4445   +/-   ##
=======================================
  Coverage   74.66%   74.66%           
=======================================
  Files         229      229           
  Lines        7568     7568           
  Branches     2834     2831    -3     
=======================================
  Hits         5651     5651           
  Misses       1660     1660           
  Partials      257      257           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a1f402...37e6633. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tkoscieln tkoscieln marked this pull request as ready for review May 18, 2026 14:07
@tkoscieln tkoscieln requested a review from a team as a code owner May 18, 2026 14:07
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • The distroId derivation assumes only whitespace differences and simple lowercase transformations; consider centralizing this logic or aligning it with the actual DOM/id generation function to avoid subtle mismatches if ids change (e.g., special characters, punctuation).
  • In ComplianceCIS.boot.ts you now hardcode label[for="rhel10-input"] while helpers.ts computes the id dynamically; consider reusing the helper logic or a shared utility to keep selection of distro inputs consistent across tests.
  • Relying on label[for="..."] selectors ties the tests to specific id attributes; if possible, prefer more stable selectors (e.g., data attributes or accessible names) to reduce brittleness when markup changes.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The distroId derivation assumes only whitespace differences and simple lowercase transformations; consider centralizing this logic or aligning it with the actual DOM/id generation function to avoid subtle mismatches if ids change (e.g., special characters, punctuation).
- In `ComplianceCIS.boot.ts` you now hardcode `label[for="rhel10-input"]` while `helpers.ts` computes the id dynamically; consider reusing the helper logic or a shared utility to keep selection of distro inputs consistent across tests.
- Relying on `label[for="..."]` selectors ties the tests to specific id attributes; if possible, prefer more stable selectors (e.g., data attributes or accessible names) to reduce brittleness when markup changes.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link
Copy Markdown
Collaborator

@mgold1234 mgold1234 left a comment

Choose a reason for hiding this comment

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

/lgtm

@tkoscieln tkoscieln enabled auto-merge May 18, 2026 14:36
@tkoscieln tkoscieln added this pull request to the merge queue May 18, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 18, 2026
@avitova avitova added this pull request to the merge queue May 19, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 19, 2026
Copy link
Copy Markdown
Collaborator

@avitova avitova left a comment

Choose a reason for hiding this comment

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

It almost seems like the getByRole for distros stopped working? Should we use getByText?
Because the failure is for different reasons than what you are fixing, it seems.

@tkoscieln
Copy link
Copy Markdown
Collaborator Author

It almost seems like the getByRole for distros stopped working? Should we use getByText? Because the failure is for different reasons than what you are fixing, it seems.

That's a different issue patternfly/patternfly-react#12422 and it has to be fixed in Patternfly patternfly/patternfly-react#12424. Since we started using Modal, I have discovered a bug where the Modal sometimes sets aria-hidden to true for dropdowns, which is what PW uses for navigation.

@tkoscieln tkoscieln added this pull request to the merge queue May 19, 2026
Merged via the queue into osbuild:main with commit ab244ca May 19, 2026
38 of 45 checks passed
@tkoscieln tkoscieln deleted the fix_compliance_distro_select branch May 19, 2026 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🍼 simple Simple and quick to review 🎭 Playwright Change in Playwright tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants