Skip to content

Conversation

@hanna-meda
Copy link
Contributor

Description

Fixes #295
Updates the “Enable all features” smoke test to type “test” into every WP Rocket text field and fail immediately if any browser validation alert appears. This helps catch regressions where built-in validation prevents saving when all options are enabled.

Type of change

  • New feature (non-breaking change which adds functionality).
  • Bug fix (non-breaking change which fixes an issue).
  • Enhancement (non-breaking change which improves an existing functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as before).
  • Sub-task of #(issue number)
  • Chore
  • Release

Detailed scenario

What was tested

Ran the “Enable all features” scenario with npm run test:test:

  • First using 3.20.1.1 (where the issue exists) as new_release ⇒ test failed
Screenshot 2025-11-14 at 00 24 48
  • Then using 3.20.1.2 (where the issue is fixed) ⇒ test passed
Screenshot 2025-11-14 at 00 15 25

Observe the step “I enable all settings with text inputs”:

  • All settings are toggled on.
  • Each text/textarea field is briefly filled with "test".
  • The scenario fails if any native browser alert appears (e.g., “Please enter a valid URL”).

How to test

Run command npm run test:smoke

Technical description

Documentation

  • Added enableAllOptionsWithTextInputs() in utils/page-utils.ts. It calls the existing enable-all helper, then validateTextInputsWithoutDialogs() which:
    • Loops through each WP Rocket section nav item.
    • For every visible editable text/textarea field (limited to safe input types), fills “test”, presses Enter, waits briefly, and restores the previous value.
    • Captures/ dismisses any Playwright dialog event and reports which field triggered it.
    • Suppresses the settings form submit while Enter is pressed by attaching a temporary listener (__wprSubmitInterceptor) so we never save unintended data.
  • Step When I enable all settings with text inputs now reuses that helper for cleaner scenarios.

New dependencies

None.

Risks

None.

Mandatory Checklist

Code validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Code style

  • I wrote a self-explanatory code about what it does.
  • I protected entry points against unexpected inputs.
  • I did not introduce unnecessary complexity.
  • Output messages (errors, notices, logs) are explicit enough for users to understand the issue and are actionnable.

Unticked items justification

If some mandatory items are not relevant, explain why in this section.

Additional Checks

  • In the case of complex code, I wrote comments to explain it.
  • When possible, I prepared ways to observe the implemented system (logs, data, etc.).
  • I added error handling logic when using functions that could throw errors (HTTP/API request, filesystem, etc.)

@hanna-meda hanna-meda linked an issue Nov 13, 2025 that may be closed by this pull request
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the "Enable all features" smoke test to validate that all text input fields in WP Rocket settings accept test values without triggering browser validation dialogs. This helps catch regressions where built-in validation could prevent saving settings when all options are enabled.

  • Adds enableAllOptionsWithTextInputs() method that enables all settings and validates text inputs
  • Implements validateTextInputsWithoutDialogs() to iterate through all sections and test text/textarea fields
  • Introduces form submit suppression during validation to prevent unintended data saves
  • Updates the smoke test scenario to use the new validation step

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
utils/page-utils.ts Adds new methods for enabling all options with text input validation, including dialog detection, form submit suppression/resumption helpers
src/support/steps/general.ts Adds new Cucumber step definition "I enable all settings with text inputs" that calls the enhanced validation method
src/features/enable-all-features.feature Updates scenario to use the new step that validates text inputs instead of just enabling settings

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@hanna-meda hanna-meda requested a review from a team November 16, 2025 21:43
Copy link
Contributor

@Miraeld Miraeld left a comment

Choose a reason for hiding this comment

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

I've made a single comment, morelike a question about one review that copilot did, other than this looks fine to me

@jeawhanlee
Copy link
Contributor

@Mai-Saad @hanna-meda So should we block this PR for the moment based on this?

@Mai-Saad
Copy link
Contributor

@jeawhanlee Agree about blocking for now. However, having 2nd thought about

What do you think if we made this with text inputs a second scenario?

What is there now is another step in the same scenario, I think this unneeded duplication and it could only be single step as it was before .. No need for new scenario / step , updating the current step to include text while enable options is enough.

@jeawhanlee
Copy link
Contributor

What is there now is another step in the same scenario, I think this unneeded duplication and it could only be single step as it was before .. No need for new scenario / step , updating the current step to include text while enable options is enough.

@Mai-Saad Just for proper test separation so one scenario is not covering 2 types of test even if they might share some things in common. Moving forward if we are to continue with the same scenario, I believe we need to update the feature name & description as well as the scenario description to be explanatory about what is being tested, otherwise we should consider splitting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update smoke test to capture wrong alert

5 participants