Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dialog rework #148

Merged
merged 2 commits into from
Feb 17, 2025
Merged

Dialog rework #148

merged 2 commits into from
Feb 17, 2025

Conversation

AlexGalichenko
Copy link
Contributor

Description

Please describe your changes and any new features you're introducing, or issues you're fixing.

  • Updated the CHANGELOG.md.
  • Updated package version in package.json, package-lock.json.
  • Checked that there aren't other open pull requests for the same issue/update.
  • Checked that your contribution follows the project's contribution guidelines.
  • Added corresponding unit/E2E tests
  • Unit and E2E pass

Related Issues

Please link any related issues or bug reports that this PR will address.

@AlexGalichenko AlexGalichenko added the enhancement New feature or request label Feb 13, 2025

```gherkin
Scenario: accept alert
Given I will wait for dialog
Copy link
Contributor

@ALegchilov ALegchilov Feb 13, 2025

Choose a reason for hiding this comment

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

To be honest it is not obvious what this step is for, future tesnse confuses a bit.
Lets consider 2 other solutions:

  1. Rename the step to Given I set alert listener
    or more elegant solution
  2. Using BeforeAll hook check if some of pickles include 'alert' keyword and then set the listener.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me listener is more programming specific. I want to use this I will wait for other types of event emitters available in playwright

Copy link
Contributor

@ALegchilov ALegchilov Feb 13, 2025

Choose a reason for hiding this comment

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

Given I expect alert will appear in this scenario

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expect already has meaning of assertion

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest I don't really like to make use of steps that dos not represent a business logic.
I know, this is not a silver bullet last option is adding a listener using a tag expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but making it implicit will add more constraints and increase support cost

@AlexGalichenko AlexGalichenko merged commit 2a46108 into main Feb 17, 2025
6 checks passed
@AlexGalichenko AlexGalichenko deleted the dialog-rework branch February 17, 2025 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants