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

added examples in docblocks of new network test steps in Playwright helper #3758

Conversation

ngraf
Copy link
Contributor

@ngraf ngraf commented Jul 12, 2023

Motivation/Description of the PR

  • I created examples in docblock of the new steps in Playwright helper for network testing.
  • Minor Functional adjustments/improvements:
    • I removed async from dontSeeTraffic, because there is no need for it. The evaluating code inside runs synchronously.
    • I make getTrafficUrl fail hard when recording of traffic has not yet started. I think this is helpful for the test automation engineer. I cannot imagine there is usecase where this new behavior is unwanted.
  • Resolves request for support from @DavertMik at feat(playwright): checking network stuff #3748 (comment)

Applicable helpers:

  • [ x ] Protractor

Type of change

  • 🔥 Breaking changes
  • 🚀 New functionality
  • 🐛 Bug fix
  • [ x ] 📋 Documentation changes/updates
  • ♨️ Hot fix
  • 🔨 Markdown files fix - not related to source code
  • [ x ] 💅 Polish code

Checklist:

  • Tests have been added
  • Documentation has been added (Run npm run docs)
  • Lint checking (Run npm run lint)
  • Local tests are passed (Run npm test)

@DavertMik
Copy link
Contributor

DavertMik commented Jul 12, 2023

@kobenguyent please merge if you think it is good
Looks overall good to me

* Examples:
*
* ```js
* await I.mockTraffic('/api/users/1', '{ id: 1, name: 'John Doe' }');
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need await here?

* Examples:
*
* ```js
* await I.blockTraffic('http://example.com/css/style.css');
Copy link
Contributor

Choose a reason for hiding this comment

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

do we always need await?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. We don't need actually.
Good catch.
I made hands-on tests and they all worked just fine without await.

-> I updated the examples in docblock. I didn't yet change it in the functional code to make the methods sync, that don't need to be async. A
ctually almost all new steps are sync methods except await I.seeTraffic
seeTraffic is async because we decided to have waiting mechanism in there for up to 10 seconds (default value).

*
* ```js
* await I.getTrafficUrl('https://api.example.com/session');
* await I.getTrafficUrl(/session.*start/);
Copy link
Contributor

Choose a reason for hiding this comment

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

I just thought, maybe we should use grab prefix?
juist to keep one standard

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 with DavertMilk.
I also would prefer "grabTrafficUrl".

@DavertMik
Copy link
Contributor

@kobenguyent please merge it

@kobenguyent kobenguyent merged commit f337b78 into codeceptjs:feat-playwright-more-custom-steps Jul 18, 2023
6 of 8 checks passed
This pull request was closed.
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.

4 participants