Skip to content

[rb] create more obvious test guard keywords as aliases#17636

Merged
titusfortner merged 4 commits into
trunkfrom
guard_specs
Jun 6, 2026
Merged

[rb] create more obvious test guard keywords as aliases#17636
titusfortner merged 4 commits into
trunkfrom
guard_specs

Conversation

@titusfortner
Copy link
Copy Markdown
Member

As we've added guard names they have become too confusing; I find I have to look them up every time and that's not great

💥 What does this PR do?

Adds four self-describing guard names for the Ruby spec suite and updates the
specs to use them:

  • skip_if: — skip the example when the configuration matches
  • skip_unless: — run the example only on the matching configuration
  • pending_if: — run, but expect failure when the configuration matches
  • pending_unless: — run, but expect failure on every configuration except the match

These map onto the existing guard behavior:

Old New Behavior
exclude skip_if skip when config matches
exclusive skip_unless run only on matching config
except pending_if pending when config matches
only pending_unless pending on every config except the match

flaky: is just skip_if with a reason required, so keep using it for
unreliable/intermittent tests.

All integration specs are converted to the new names except those marked flaky.

rb/AGENTS.md documents the guards, the matching semantics (a Hash is "and",
an Array of Hashes is "or"), that guards apply at the describe/context/it
levels, and points to the active conditions in rb/spec/integration/selenium/webdriver/spec_helper.rb.

🔧 Implementation Notes

  • New types are aliasing old types, so any other usage stays the same
  • Not deprecating old methods

🤖 AI assistance

  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: guard alias implementation, spec conversion, docs, and tests
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • Cleanup (formatting, renaming)

@selenium-ci selenium-ci added C-rb Ruby Bindings B-devtools Includes everything BiDi or Chrome DevTools related B-support Issue or PR related to support classes labels Jun 5, 2026
@selenium-ci
Copy link
Copy Markdown
Member

Thank you, @titusfortner for this code suggestion.

The support packages contain example code that many users find helpful, but they do not necessarily represent
the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium
to work, we will likely close the PR.

We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
If you have any questions, please contact us

@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Add skip_if/skip_unless/pending_if/pending_unless guard aliases for Ruby specs

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• Add self-describing guard aliases for Ruby specs
  - skip_if and skip_unless for skipping tests
  - pending_if and pending_unless for expecting failures
• Update all integration specs to use new guard names
• Document guard semantics and matching behavior in AGENTS.md
• Maintain backward compatibility with old guard names
Diagram
flowchart LR
  A["Old Guard Names<br/>exclude, exclusive, except, only"] -->|"Aliased to"| B["New Guard Names<br/>skip_if, skip_unless,<br/>pending_if, pending_unless"]
  B -->|"Applied to"| C["Integration Specs<br/>describe/context/it blocks"]
  D["Guard Implementation<br/>guards.rb"] -->|"Updated with"| E["Alias Logic<br/>Type Checking"]
  F["Documentation<br/>AGENTS.md"] -->|"Explains"| B

Loading

Grey Divider

File Changes

1. rb/lib/selenium/webdriver/support/guards.rb ✨ Enhancement +2/-1

Add new guard types to GUARD_TYPES constant

rb/lib/selenium/webdriver/support/guards.rb


2. rb/lib/selenium/webdriver/support/guards/guard.rb ✨ Enhancement +10/-11

Update guard type checking to support aliases

rb/lib/selenium/webdriver/support/guards/guard.rb


3. rb/spec/integration/selenium/webdriver/action_builder_spec.rb 🧪 Tests +12/-12

Convert guards to new skip_if/skip_unless/pending_if names

rb/spec/integration/selenium/webdriver/action_builder_spec.rb


View more (38)
4. rb/spec/integration/selenium/webdriver/bidi/browser_spec.rb 🧪 Tests +2/-2

Convert exclusive/only guards to skip_unless/pending_unless

rb/spec/integration/selenium/webdriver/bidi/browser_spec.rb


5. rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb 🧪 Tests +10/-9

Convert guards to new self-describing alias names

rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb


6. rb/spec/integration/selenium/webdriver/bidi/network_spec.rb 🧪 Tests +4/-4

Convert exclusive/only guards to new alias names

rb/spec/integration/selenium/webdriver/bidi/network_spec.rb


7. rb/spec/integration/selenium/webdriver/bidi/script_spec.rb 🧪 Tests +2/-2

Convert exclusive/only guards to skip_unless/pending_unless

rb/spec/integration/selenium/webdriver/bidi/script_spec.rb


8. rb/spec/integration/selenium/webdriver/bidi_spec.rb 🧪 Tests +2/-2

Convert exclusive/only guards to new alias names

rb/spec/integration/selenium/webdriver/bidi_spec.rb


9. rb/spec/integration/selenium/webdriver/chrome/driver_spec.rb 🧪 Tests +1/-1

Convert exclusive guard to skip_unless

rb/spec/integration/selenium/webdriver/chrome/driver_spec.rb


10. rb/spec/integration/selenium/webdriver/chrome/options_spec.rb 🧪 Tests +3/-3

Convert exclusive guards to skip_unless

rb/spec/integration/selenium/webdriver/chrome/options_spec.rb


11. rb/spec/integration/selenium/webdriver/chrome/service_spec.rb 🧪 Tests +2/-2

Convert exclude/exclusive to skip_if/skip_unless

rb/spec/integration/selenium/webdriver/chrome/service_spec.rb


12. rb/spec/integration/selenium/webdriver/devtools_spec.rb 🧪 Tests +3/-3

Convert exclusive/except guards to new names

rb/spec/integration/selenium/webdriver/devtools_spec.rb


13. rb/spec/integration/selenium/webdriver/driver_finder_spec.rb 🧪 Tests +4/-4

Convert exclude/except/exclusive guards to new names

rb/spec/integration/selenium/webdriver/driver_finder_spec.rb


14. rb/spec/integration/selenium/webdriver/driver_spec.rb 🧪 Tests +6/-6

Convert exclude/except/exclusive guards to new names

rb/spec/integration/selenium/webdriver/driver_spec.rb


15. rb/spec/integration/selenium/webdriver/edge/driver_spec.rb 🧪 Tests +1/-1

Convert exclusive guard to skip_unless

rb/spec/integration/selenium/webdriver/edge/driver_spec.rb


16. rb/spec/integration/selenium/webdriver/edge/options_spec.rb 🧪 Tests +3/-3

Convert exclusive guards to skip_unless

rb/spec/integration/selenium/webdriver/edge/options_spec.rb


17. rb/spec/integration/selenium/webdriver/edge/service_spec.rb 🧪 Tests +2/-2

Convert exclude/exclusive to skip_if/skip_unless

rb/spec/integration/selenium/webdriver/edge/service_spec.rb


18. rb/spec/integration/selenium/webdriver/element_spec.rb 🧪 Tests +14/-14

Convert exclusive/except guards to new names

rb/spec/integration/selenium/webdriver/element_spec.rb


19. rb/spec/integration/selenium/webdriver/error_spec.rb 🧪 Tests +2/-2

Convert exclusive guard to skip_unless

rb/spec/integration/selenium/webdriver/error_spec.rb


20. rb/spec/integration/selenium/webdriver/fedcm_spec.rb 🧪 Tests +2/-2

Convert exclusive/except guards to new names

rb/spec/integration/selenium/webdriver/fedcm_spec.rb


21. rb/spec/integration/selenium/webdriver/firefox/driver_spec.rb 🧪 Tests +6/-6

Convert exclusive/except guards to new names

rb/spec/integration/selenium/webdriver/firefox/driver_spec.rb


22. rb/spec/integration/selenium/webdriver/firefox/profile_spec.rb 🧪 Tests +2/-2

Convert exclusive/exclude guards to new names

rb/spec/integration/selenium/webdriver/firefox/profile_spec.rb


23. rb/spec/integration/selenium/webdriver/firefox/service_spec.rb 🧪 Tests +2/-2

Convert exclude/exclusive to skip_if/skip_unless

rb/spec/integration/selenium/webdriver/firefox/service_spec.rb


24. rb/spec/integration/selenium/webdriver/ie/service_spec.rb 🧪 Tests +2/-2

Convert exclude/exclusive to skip_if/skip_unless

rb/spec/integration/selenium/webdriver/ie/service_spec.rb


25. rb/spec/integration/selenium/webdriver/listener_spec.rb 🧪 Tests +1/-1

Convert exclusive guard to skip_unless

rb/spec/integration/selenium/webdriver/listener_spec.rb


26. rb/spec/integration/selenium/webdriver/manager_spec.rb 🧪 Tests +18/-18

Convert exclusive/except/only guards to new names

rb/spec/integration/selenium/webdriver/manager_spec.rb


27. rb/spec/integration/selenium/webdriver/network_spec.rb 🧪 Tests +3/-3

Convert exclusive/except guards to new names

rb/spec/integration/selenium/webdriver/network_spec.rb


28. rb/spec/integration/selenium/webdriver/remote/driver_spec.rb 🧪 Tests +5/-5

Convert exclusive/exclude guards to new names

rb/spec/integration/selenium/webdriver/remote/driver_spec.rb


29. rb/spec/integration/selenium/webdriver/remote/element_spec.rb 🧪 Tests +7/-9

Convert exclusive guard to skip_unless

rb/spec/integration/selenium/webdriver/remote/element_spec.rb


30. rb/spec/integration/selenium/webdriver/safari/driver_spec.rb 🧪 Tests +4/-4

Convert exclusive guard to skip_unless

rb/spec/integration/selenium/webdriver/safari/driver_spec.rb


31. rb/spec/integration/selenium/webdriver/safari/service_spec.rb 🧪 Tests +2/-2

Convert exclude/exclusive to skip_if/skip_unless

rb/spec/integration/selenium/webdriver/safari/service_spec.rb


32. rb/spec/integration/selenium/webdriver/select_spec.rb 🧪 Tests +8/-8

Convert exclusive/exclude guards to new names

rb/spec/integration/selenium/webdriver/select_spec.rb


33. rb/spec/integration/selenium/webdriver/shadow_root_spec.rb 🧪 Tests +13/-13

Convert exclusive/except guards to new names

rb/spec/integration/selenium/webdriver/shadow_root_spec.rb


34. rb/spec/integration/selenium/webdriver/takes_screenshot_spec.rb 🧪 Tests +7/-6

Convert exclusive/except/only guards to new names

rb/spec/integration/selenium/webdriver/takes_screenshot_spec.rb


35. rb/spec/integration/selenium/webdriver/target_locator_spec.rb 🧪 Tests +3/-3

Convert except guard to pending_if

rb/spec/integration/selenium/webdriver/target_locator_spec.rb


36. rb/spec/integration/selenium/webdriver/timeout_spec.rb 🧪 Tests +2/-2

Convert except guard to pending_if

rb/spec/integration/selenium/webdriver/timeout_spec.rb


37. rb/spec/integration/selenium/webdriver/virtual_authenticator_spec.rb 🧪 Tests +2/-2

Convert exclusive guard to skip_unless

rb/spec/integration/selenium/webdriver/virtual_authenticator_spec.rb


38. rb/spec/integration/selenium/webdriver/window_spec.rb 🧪 Tests +5/-5

Convert except/exclude guards to new names

rb/spec/integration/selenium/webdriver/window_spec.rb


39. rb/spec/unit/selenium/webdriver/guard_spec.rb 🧪 Tests +26/-2

Add tests for new guard alias types

rb/spec/unit/selenium/webdriver/guard_spec.rb


40. rb/spec/unit/selenium/webdriver/guards_spec.rb 🧪 Tests +67/-23

Update tests to use new guard names and add backward compatibility tests

rb/spec/unit/selenium/webdriver/guards_spec.rb


41. rb/AGENTS.md 📝 Documentation +20/-0

Document integration test guards and matching semantics

rb/AGENTS.md


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 5, 2026

Code Review by Qodo

🐞 Bugs (5) 📘 Rule violations (2)

Grey Divider


Action required

1. Wrong guard array semantics 🐞 Bug ≡ Correctness
Description
rb/AGENTS.md claims “Array of Hashes = OR (any matches)”, but the guard engine flattens arrays into
multiple Guard instances and, for skip_unless/pending_unless, treats them as an AND (any unsatisfied
guard triggers skip/pending). Following the docs can cause examples to be skipped/pended in
configurations they were intended to run, silently reducing coverage.
Code

rb/AGENTS.md[R64-66]

+Matching: Hash = AND (all pairs match), Array of Hashes = OR (any matches), Array value = OR within a
+key. The `reason:` is a String or an issue number. Conditions (`browser`, `driver`, `platform`, `ci`,
+etc.) are registered in [`spec_helper.rb`](spec/integration/selenium/webdriver/spec_helper.rb).
Evidence
The docs promise OR semantics for an Array of Hashes, but the implementation flattens arrays into
separate guards and then applies skip_unless/pending_unless by triggering on the first
*unsatisfied* guard, which requires all entries to match (AND). Repo usage already passes arrays to
skip_unless, so incorrect docs can directly lead to mis-guarded tests.

rb/AGENTS.md[64-66]
rb/lib/selenium/webdriver/support/guards.rb[67-84]
rb/lib/selenium/webdriver/support/guards.rb[86-94]
rb/spec/integration/selenium/webdriver/devtools_spec.rb[24-25]
rb/spec/unit/selenium/webdriver/guards_spec.rb[112-116]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`rb/AGENTS.md` documents guard matching as “Hash = AND, Array of Hashes = OR”, but the actual implementation only behaves like OR for *positive* guards (`skip_if`/`pending_if`). For *negative* guards (`skip_unless`/`pending_unless`), an Array of Hashes is expanded into multiple guards and effectively combined as AND (all must match), since any non-match triggers skip/pending.

### Issue Context
This mismatch is especially risky because the suite already uses `skip_unless` with arrays (e.g., combining `bidi` and `browser` requirements). Developers relying on the new docs may author guards that behave differently than expected.

### Fix Focus Areas
- rb/AGENTS.md[64-66]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Misleading skip_if message 🐞 Bug ◔ Observability
Description
Guards::Guard#message prints skip_if skips as “breaks test run”, but skip_if is documented and
used for general config-based skips (e.g., Safari-specific behavior differences). This makes skip
output misleading and harder to debug because the message implies a stronger condition than the
guard usage.
Code

rb/lib/selenium/webdriver/support/guards/guard.rb[R53-60]

            case type
-            when :exclude
+            when :skip_if, :exclude
              "Test skipped because it breaks test run; #{details}"
            when :flaky
              "Test skipped because it is unreliable in this configuration; #{details}"
-            when :exclusive
+            when :skip_unless, :exclusive
              "Test does not apply to this configuration; #{details}"
            else
Evidence
Documentation and updated spec usages treat skip_if as a general conditional skip, while
Guard#message hardcodes a run-breaking rationale for skip_if, so the emitted skip reason text
does not match intended/actual usage.

rb/AGENTS.md[53-62]
rb/lib/selenium/webdriver/support/guards/guard.rb[43-62]
rb/spec/integration/selenium/webdriver/driver_spec.rb[149-155]
rb/spec/integration/selenium/webdriver/select_spec.rb[113-118]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`skip_if` is intended as a general “skip when config matches” guard, but `Guard#message` currently formats it as “Test skipped because it breaks test run”, which is inaccurate for many `skip_if` usages (e.g., browser-specific behavior gaps).

### Issue Context
The PR introduces `skip_if` as a self-describing alias and converts many specs to use it. Several converted specs skip for reasons that do not “break the test run” (e.g., Safari raises a different exception). The message should reflect general conditional skipping.

### Fix Focus Areas
- rb/lib/selenium/webdriver/support/guards/guard.rb[53-62]
- rb/spec/unit/selenium/webdriver/guard_spec.rb[199-206]

### Suggested change
1. Split the message handling so `:skip_if` has a more generic message, while keeping the legacy `:exclude` wording if desired for backwards compatibility, e.g.:
  - `when :skip_if` -> `"Test skipped in this configuration; #{details}"`
  - `when :exclude` -> keep `"Test skipped because it breaks test run; #{details}"`
2. Update/adjust unit specs that currently assert the old `skip_if` message string.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Reason type docs incomplete 🐞 Bug ⚙ Maintainability
Description
rb/AGENTS.md states reason: is “a String or an issue number”, but the guard implementation also
supports Symbol reasons that map through Guards#add_message, so the docs hide supported behavior
and can mislead spec authors.
Code

rb/AGENTS.md[68]

+skipped/pended if any does not). The `reason:` is a String or an issue number. Conditions (`browser`,
Evidence
The documentation claims reason: is only String/issue-number, but the guard message formatting has
a dedicated Symbol branch and unit tests demonstrate Symbol reasons and add_message usage, proving
this is supported behavior.

rb/lib/selenium/webdriver/support/guards/guard.rb[43-51]
rb/spec/unit/selenium/webdriver/guard_spec.rb[62-69]
rb/spec/unit/selenium/webdriver/guard_spec.rb[185-197]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`rb/AGENTS.md` currently documents `reason:` as only a String or issue number, but the Ruby guard implementation also accepts `reason:` as a Symbol which is resolved via the guard message registry (`Guards#add_message`).

### Issue Context
`Selenium::WebDriver::Support::Guards::Guard#message` has explicit handling for `reason` being an `Integer` or a `Symbol` (not just String/Integer), and unit tests cover Symbol reasons.

### Fix Focus Areas
- rb/AGENTS.md[64-70]

### Suggested doc adjustment
Update the sentence to something like:
- "The `reason:` is a String, an issue number (Integer), or a Symbol key registered via `Guards#add_message`."

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. BiDi specs over-selected 🐞 Bug ≡ Correctness
Description
BiDi spec groups were changed to only skip_unless: {bidi: true}, but :bidi is derived solely
from ENV['WEBDRIVER_BIDI'], not from the current browser/driver. When that env var is set, these
examples run on any configured browser and then unconditionally call driver.bidi, which will fail
on browsers/drivers not configured for BiDi in this suite.
Code

rb/spec/integration/selenium/webdriver/bidi_spec.rb[R24-25]

+    describe BiDi, skip_unless: {bidi: true, reason: 'only executed when bidi is enabled'} do
      after { |example| reset_driver!(example: example) }
Evidence
:bidi is enabled solely by ENV['WEBDRIVER_BIDI'], so skip_unless: {bidi: true} no longer
limits execution by browser. The BiDi specs then call driver.bidi in regular (non-“bidi not
enabled”) examples, while the test environment only wires BiDi-specific options for
Chrome/Edge/Firefox when that env var is set.

rb/spec/integration/selenium/webdriver/spec_helper.rb[72-85]
rb/spec/integration/selenium/webdriver/bidi_spec.rb[24-37]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[342-371]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
BiDi integration specs removed the previous browser guard and now only gate on `bidi: true`. Since `:bidi` is set purely from `ENV['WEBDRIVER_BIDI']`, these specs will execute on any configured browser whenever that env var is present, and they call `driver.bidi` in normal examples.

## Issue Context
The suite’s BiDi enablement is not universally configured for all browsers; it’s toggled via env var and option wiring, so running BiDi specs on a non-BiDi browser/driver configuration becomes a hard failure.

## Fix Focus Areas
Re-introduce the browser restriction for these BiDi spec groups (either by restoring the prior `pending_unless`/`only`-equivalent guard, or by adding `browser:` to the `skip_unless` hash) across all BiDi spec entry points.

- rb/spec/integration/selenium/webdriver/bidi_spec.rb[24-24]
- rb/spec/integration/selenium/webdriver/bidi/browser_spec.rb[25-25]
- rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb[25-25]
- rb/spec/integration/selenium/webdriver/bidi/network_spec.rb[25-25]
- rb/spec/integration/selenium/webdriver/bidi/script_spec.rb[24-24]
- rb/spec/integration/selenium/webdriver/network_spec.rb[24-24]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (3)
5. Reason requirement contradicts code 🐞 Bug ⚙ Maintainability
Description
rb/AGENTS.md states guards should “always include a reason:”, but the guard implementation accepts
missing reasons and injects "No reason given", and multiple specs use guards without a reason. This
contradiction reduces the usefulness of skip/pending output and makes the new documentation
unreliable.
Code

rb/AGENTS.md[R55-57]

+Integration specs are guarded by RSpec metadata on `describe`/`context`/`it` (all enclosing
+guards combine). Use one of these five, and always include a `reason:`:
+
Evidence
The documentation introduces a hard requirement for reason:, but Guard initialization explicitly
defaults missing reason values, and the suite contains guard metadata without reasons, so the doc
statement is not true in practice.

rb/AGENTS.md[55-57]
rb/lib/selenium/webdriver/support/guards/guard.rb[32-41]
rb/spec/integration/selenium/webdriver/chrome/options_spec.rb[22-26]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`rb/AGENTS.md` says guard metadata should “always include a `reason:`”, but the implementation does not enforce this and will default to “No reason given”. Several specs also omit `reason:` on guards, so the new guidance is currently inconsistent with the codebase.

### Issue Context
If the intent is to require reasons, enforce it (at least for new guard names) or update all guard usage. If not, adjust the docs to say `reason:` is recommended (or required only for `flaky:`).

### Fix Focus Areas
- rb/AGENTS.md[55-57]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Broad guard rename churn 📘 Rule violation ⚙ Maintainability
Description
This PR performs a large, repo-wide mechanical rename of RSpec guard metadata (e.g.,
exclusive/exclude -> skip_unless/skip_if) across many integration specs even though
backward-compatible aliases are added, increasing review risk and merge conflict probability.
Code

rb/spec/integration/selenium/webdriver/action_builder_spec.rb[24]

+    describe ActionBuilder, skip_unless: {bidi: false, reason: 'Not yet implemented with BiDi'} do
Evidence
PR Compliance ID 3 forbids broad repo-wide refactors/renames not required for the functional change.
The diff shows many mechanical guard-key renames across multiple integration spec files despite the
implementation keeping the old guard types as aliases, indicating avoidable churn.

AGENTS.md: Avoid Repository-Wide Refactors or Formatting-Only Churn
rb/spec/integration/selenium/webdriver/action_builder_spec.rb[24-24]
rb/spec/integration/selenium/webdriver/devtools_spec.rb[24-25]
rb/spec/integration/selenium/webdriver/driver_spec.rb[27-31]
rb/spec/integration/selenium/webdriver/window_spec.rb[116-130]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR introduces broad mechanical changes across many spec files (guard key renames) that are not strictly required for functionality because compatibility aliases exist.

## Issue Context
Compliance asks to avoid repository-wide refactors/churn to keep PRs reviewable and low-risk.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/action_builder_spec.rb[24-24]
- rb/spec/integration/selenium/webdriver/devtools_spec.rb[24-25]
- rb/spec/integration/selenium/webdriver/driver_spec.rb[27-31]
- rb/spec/integration/selenium/webdriver/window_spec.rb[116-130]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


7. Guard comments restate behavior 📘 Rule violation ⚙ Maintainability
Description
The modified comments in Guard explain what each predicate does but not why the semantics/aliases
exist, reducing maintainability and reviewer understanding of the design intent.
Code

rb/lib/selenium/webdriver/support/guards/guard.rb[R65-83]

+          # Test is expected to fail on the configurations specified (marked pending).
          def except?
-            @type == :except
+            @type == :pending_if || @type == :except
          end

-          # Bug is present on all configurations not specified
+          # Test is expected to fail on every configuration except those specified (marked pending).
          def only?
-            @type == :only
+            @type == :pending_unless || @type == :only
          end

-          # Bug is present on all configurations specified, but test can not be run because it breaks other tests,
-          # or it is flaky and unreliable
+          # Test is skipped on the configurations specified because it breaks the run or is unreliable.
          def exclude?
-            @type == :exclude || @type == :flaky
+            @type == :skip_if || @type == :exclude || @type == :flaky
          end

-          # Test only applies to configurations specified
+          # Test is skipped on every configuration except those specified (it only applies there).
          def exclusive?
-            @type == :exclusive
+            @type == :skip_unless || @type == :exclusive
          end
Evidence
PR Compliance ID 6 requires comments to explain rationale (“why”), not restate behavior (“what”).
The changed comment blocks describe the direct behavior of except?/only?/exclude?/exclusive?
rather than documenting the reason for the alias mapping and intended semantics.

AGENTS.md: Use Comments to Explain Why, Not What
rb/lib/selenium/webdriver/support/guards/guard.rb[65-83]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Updated comments restate observable behavior rather than explaining intent/rationale (e.g., why `pending_if`/`pending_unless` are treated as aliases for `except`/`only`).

## Issue Context
Compliance requires comments to focus on non-obvious rationale/constraints instead of describing what the code already says.

## Fix Focus Areas
- rb/lib/selenium/webdriver/support/guards/guard.rb[65-83]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit 1149d56

Results up to commit 3964f2f


🐞 Bugs (2) 📘 Rule violations (2) 📎 Requirement gaps (0) 🎨 UX issues (0)


Action required
1. Wrong guard array semantics 🐞 Bug ≡ Correctness
Description
rb/AGENTS.md claims “Array of Hashes = OR (any matches)”, but the guard engine flattens arrays into
multiple Guard instances and, for skip_unless/pending_unless, treats them as an AND (any unsatisfied
guard triggers skip/pending). Following the docs can cause examples to be skipped/pended in
configurations they were intended to run, silently reducing coverage.
Code

rb/AGENTS.md[R64-66]

+Matching: Hash = AND (all pairs match), Array of Hashes = OR (any matches), Array value = OR within a
+key. The `reason:` is a String or an issue number. Conditions (`browser`, `driver`, `platform`, `ci`,
+etc.) are registered in [`spec_helper.rb`](spec/integration/selenium/webdriver/spec_helper.rb).
Evidence
The docs promise OR semantics for an Array of Hashes, but the implementation flattens arrays into
separate guards and then applies skip_unless/pending_unless by triggering on the first
*unsatisfied* guard, which requires all entries to match (AND). Repo usage already passes arrays to
skip_unless, so incorrect docs can directly lead to mis-guarded tests.

rb/AGENTS.md[64-66]
rb/lib/selenium/webdriver/support/guards.rb[67-84]
rb/lib/selenium/webdriver/support/guards.rb[86-94]
rb/spec/integration/selenium/webdriver/devtools_spec.rb[24-25]
rb/spec/unit/selenium/webdriver/guards_spec.rb[112-116]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`rb/AGENTS.md` documents guard matching as “Hash = AND, Array of Hashes = OR”, but the actual implementation only behaves like OR for *positive* guards (`skip_if`/`pending_if`). For *negative* guards (`skip_unless`/`pending_unless`), an Array of Hashes is expanded into multiple guards and effectively combined as AND (all must match), since any non-match triggers skip/pending.

### Issue Context
This mismatch is especially risky because the suite already uses `skip_unless` with arrays (e.g., combining `bidi` and `browser` requirements). Developers relying on the new docs may author guards that behave differently than expected.

### Fix Focus Areas
- rb/AGENTS.md[64-66]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended
2. Broad guard rename churn 📘 Rule violation ⚙ Maintainability
Description
This PR performs a large, repo-wide mechanical rename of RSpec guard metadata (e.g.,
exclusive/exclude -> skip_unless/skip_if) across many integration specs even though
backward-compatible aliases are added, increasing review risk and merge conflict probability.
Code

rb/spec/integration/selenium/webdriver/action_builder_spec.rb[24]

+    describe ActionBuilder, skip_unless: {bidi: false, reason: 'Not yet implemented with BiDi'} do
Evidence
PR Compliance ID 3 forbids broad repo-wide refactors/renames not required for the functional change.
The diff shows many mechanical guard-key renames across multiple integration spec files despite the
implementation keeping the old guard types as aliases, indicating avoidable churn.

AGENTS.md: Avoid Repository-Wide Refactors or Formatting-Only Churn
rb/spec/integration/selenium/webdriver/action_builder_spec.rb[24-24]
rb/spec/integration/selenium/webdriver/devtools_spec.rb[24-25]
rb/spec/integration/selenium/webdriver/driver_spec.rb[27-31]
rb/spec/integration/selenium/webdriver/window_spec.rb[116-130]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR introduces broad mechanical changes across many spec files (guard key renames) that are not strictly required for functionality because compatibility aliases exist.

## Issue Context
Compliance asks to avoid repository-wide refactors/churn to keep PRs reviewable and low-risk.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/action_builder_spec.rb[24-24]
- rb/spec/integration/selenium/webdriver/devtools_spec.rb[24-25]
- rb/spec/integration/selenium/webdriver/driver_spec.rb[27-31]
- rb/spec/integration/selenium/webdriver/window_spec.rb[116-130]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Guard comments restate behavior 📘 Rule violation ⚙ Maintainability
Description
The modified comments in Guard explain what each predicate does but not why the semantics/aliases
exist, reducing maintainability and reviewer understanding of the design intent.
Code

rb/lib/selenium/webdriver/support/guards/guard.rb[R65-83]

+          # Test is expected to fail on the configurations specified (marked pending).
          def except?
-            @type == :except
+            @type == :pending_if || @type == :except
          end

-          # Bug is present on all configurations not specified
+          # Test is expected to fail on every configuration except those specified (marked pending).
          def only?
-            @type == :only
+            @type == :pending_unless || @type == :only
          end

-          # Bug is present on all configurations specified, but test can not be run because it breaks other tests,
-          # or it is flaky and unreliable
+          # Test is skipped on the configurations specified because it breaks the run or is unreliable.
          def exclude?
-            @type == :exclude || @type == :flaky
+            @type == :skip_if || @type == :exclude || @type == :flaky
          end

-          # Test only applies to configurations specified
+          # Test is skipped on every configuration except those specified (it only applies there).
          def exclusive?
-            @type == :exclusive
+            @type == :skip_unless || @type == :exclusive
          end
Evidence
PR Compliance ID 6 requires comments to explain rationale (“why”), not restate behavior (“what”).
The changed comment blocks describe the direct behavior of except?/only?/exclude?/exclusive?
rather than documenting the reason for the alias mapping and intended semantics.

AGENTS.md: Use Comments to Explain Why, Not What
rb/lib/selenium/webdriver/support/guards/guard.rb[65-83]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Updated comments restate observable behavior rather than explaining intent/rationale (e.g., why `pending_if`/`pending_unless` are treated as aliases for `except`/`only`).

## Issue Context
Compliance requires comments to focus on non-obvious rationale/constraints instead of describing what the code already says.

## Fix Focus Areas
- rb/lib/selenium/webdriver/support/guards/guard.rb[65-83]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Reason requirement contradicts code 🐞 Bug ⚙ Maintainability
Description
rb/AGENTS.md states guards should “always include a reason:”, but the guard implementation accepts
missing reasons and injects "No reason given", and multiple specs use guards without a reason. This
contradiction reduces the usefulness of skip/pending output and makes the new documentation
unreliable.
Code

rb/AGENTS.md[R55-57]

+Integration specs are guarded by RSpec metadata on `describe`/`context`/`it` (all enclosing
+guards combine). Use one of these five, and always include a `reason:`:
+
Evidence
The documentation introduces a hard requirement for reason:, but Guard initialization explicitly
defaults missing reason values, and the suite contains guard metadata without reasons, so the doc
statement is not true in practice.

rb/AGENTS.md[55-57]
rb/lib/selenium/webdriver/support/guards/guard.rb[32-41]
rb/spec/integration/selenium/webdriver/chrome/options_spec.rb[22-26]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`rb/AGENTS.md` says guard metadata should “always include a `reason:`”, but the implementation does not enforce this and will default to “No reason given”. Several specs also omit `reason:` on guards, so the new guidance is currently inconsistent with the codebase.

### Issue Context
If the intent is to require reasons, enforce it (at least for new guard names) or update all guard usage. If not, adjust the docs to say `reason:` is recommended (or required only for `flaky:`).

### Fix Focus Areas
- rb/AGENTS.md[55-57]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 6fbfe43


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0)


Remediation recommended
1. BiDi specs over-selected 🐞 Bug ≡ Correctness
Description
BiDi spec groups were changed to only skip_unless: {bidi: true}, but :bidi is derived solely
from ENV['WEBDRIVER_BIDI'], not from the current browser/driver. When that env var is set, these
examples run on any configured browser and then unconditionally call driver.bidi, which will fail
on browsers/drivers not configured for BiDi in this suite.
Code

rb/spec/integration/selenium/webdriver/bidi_spec.rb[R24-25]

+    describe BiDi, skip_unless: {bidi: true, reason: 'only executed when bidi is enabled'} do
      after { |example| reset_driver!(example: example) }
Evidence
:bidi is enabled solely by ENV['WEBDRIVER_BIDI'], so skip_unless: {bidi: true} no longer
limits execution by browser. The BiDi specs then call driver.bidi in regular (non-“bidi not
enabled”) examples, while the test environment only wires BiDi-specific options for
Chrome/Edge/Firefox when that env var is set.

rb/spec/integration/selenium/webdriver/spec_helper.rb[72-85]
rb/spec/integration/selenium/webdriver/bidi_spec.rb[24-37]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[342-371]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
BiDi integration specs removed the previous browser guard and now only gate on `bidi: true`. Since `:bidi` is set purely from `ENV['WEBDRIVER_BIDI']`, these specs will execute on any configured browser whenever that env var is present, and they call `driver.bidi` in normal examples.

## Issue Context
The suite’s BiDi enablement is not universally configured for all browsers; it’s toggled via env var and option wiring, so running BiDi specs on a non-BiDi browser/driver configuration becomes a hard failure.

## Fix Focus Areas
Re-introduce the browser restriction for these BiDi spec groups (either by restoring the prior `pending_unless`/`only`-equivalent guard, or by adding `browser:` to the `skip_unless` hash) across all BiDi spec entry points.

- rb/spec/integration/selenium/webdriver/bidi_spec.rb[24-24]
- rb/spec/integration/selenium/webdriver/bidi/browser_spec.rb[25-25]
- rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb[25-25]
- rb/spec/integration/selenium/webdriver/bidi/network_spec.rb[25-25]
- rb/spec/integration/selenium/webdriver/bidi/script_spec.rb[24-24]
- rb/spec/integration/selenium/webdriver/network_spec.rb[24-24]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit a3a99f3


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0)


Remediation recommended
1. Reason type docs incomplete 🐞 Bug ⚙ Maintainability
Description
rb/AGENTS.md states reason: is “a String or an issue number”, but the guard implementation also
supports Symbol reasons that map through Guards#add_message, so the docs hide supported behavior
and can mislead spec authors.
Code

rb/AGENTS.md[68]

+skipped/pended if any does not). The `reason:` is a String or an issue number. Conditions (`browser`,
Evidence
The documentation claims reason: is only String/issue-number, but the guard message formatting has
a dedicated Symbol branch and unit tests demonstrate Symbol reasons and add_message usage, proving
this is supported behavior.

rb/lib/selenium/webdriver/support/guards/guard.rb[43-51]
rb/spec/unit/selenium/webdriver/guard_spec.rb[62-69]
rb/spec/unit/selenium/webdriver/guard_spec.rb[185-197]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`rb/AGENTS.md` currently documents `reason:` as only a String or issue number, but the Ruby guard implementation also accepts `reason:` as a Symbol which is resolved via the guard message registry (`Guards#add_message`).

### Issue Context
`Selenium::WebDriver::Support::Guards::Guard#message` has explicit handling for `reason` being an `Integer` or a `Symbol` (not just String/Integer), and unit tests cover Symbol reasons.

### Fix Focus Areas
- rb/AGENTS.md[64-70]

### Suggested doc adjustment
Update the sentence to something like:
- "The `reason:` is a String, an issue number (Integer), or a Symbol key registered via `Guards#add_message`."

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 4217a7c


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0)


Remediation recommended
1. Misleading skip_if message 🐞 Bug ◔ Observability
Description
Guards::Guard#message prints skip_if skips as “breaks test run”, but skip_if is documented and
used for general config-based skips (e.g., Safari-specific behavior differences). This makes skip
output misleading and harder to debug because the message implies a stronger condition than the
guard usage.
Code

rb/lib/selenium/webdriver/support/guards/guard.rb[R53-60]

            case type
-            when :exclude
+            when :skip_if, :exclude
              "Test skipped because it breaks test run; #{details}"
            when :flaky
              "Test skipped because it is unreliable in this configuration; #{details}"
-            when :exclusive
+            when :skip_unless, :exclusive
              "Test does not apply to this configuration; #{details}"
            else
Evidence
Documentation and updated spec usages treat skip_if as a general conditional skip, while
Guard#message hardcodes a run-breaking rationale for skip_if, so the emitted skip reason text
does not match intended/actual usage.

rb/AGENTS.md[53-62]
rb/lib/selenium/webdriver/support/guards/guard.rb[43-62]
rb/spec/integration/selenium/webdriver/driver_spec.rb[149-155]
rb/spec/integration/selenium/webdriver/select_spec.rb[113-118]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`skip_if` is intended as a general “skip when config matches” guard, but `Guard#message` currently formats it as “Test skipped because it breaks test run”, which is inaccurate for many `skip_if` usages (e.g., browser-specific behavior gaps).

### Issue Context
The PR introduces `skip_if` as a self-describing alias and converts many specs to use it. Several converted specs skip for reasons that do not “break the test run” (e.g., Safari raises a different exception). The message should reflect general conditional skipping.

### Fix Focus Areas
- rb/lib/selenium/webdriver/support/guards/guard.rb[53-62]
- rb/spec/unit/selenium/webdriver/guard_spec.rb[199-206]

### Suggested change
1. Split the message handling so `:skip_if` has a more generic message, while keeping the legacy `:exclude` wording if desired for backwards compatibility, e.g.:
  - `when :skip_if` -> `"Test skipped in this configuration; #{details}"`
  - `when :exclude` -> keep `"Test skipped because it breaks test run; #{details}"`
2. Update/adjust unit specs that currently assert the old `skip_if` message string.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

@titusfortner titusfortner requested a review from aguspe June 5, 2026 15:49
@titusfortner titusfortner changed the title [rb] add skip_if/skip_unless/pending_if/pending_unless guard aliases [rb] create more obvious test guard keywords as aliases Jun 5, 2026
Comment thread rb/AGENTS.md Outdated
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 5, 2026

Code review by qodo was updated up to the latest commit 6fbfe43

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 5, 2026

Code review by qodo was updated up to the latest commit a3a99f3

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 5, 2026

Code review by qodo was updated up to the latest commit 4217a7c

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Jun 6, 2026

Code review by qodo was updated up to the latest commit 1149d56

@titusfortner titusfortner merged commit bf0edc8 into trunk Jun 6, 2026
31 checks passed
@titusfortner titusfortner deleted the guard_specs branch June 6, 2026 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-devtools Includes everything BiDi or Chrome DevTools related B-support Issue or PR related to support classes C-rb Ruby Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants