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.

Sign up for GitHub

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

docs(contributing): add info about skip and fail #1944

Merged
merged 1 commit into from
Apr 23, 2020
Merged

docs(contributing): add info about skip and fail #1944

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,26 @@ HEADLESS=false SLOW_MO=500 npm run wtest
BROWSER=chromium node --inspect-brk test/test.js
```

- When should a test be marked with `skip` or `fail`?
Copy link
Member Author

Choose a reason for hiding this comment

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

I did not comment about how these interact with CI/CD of an MR passing/failing. It may be helpful to add that, too.


- **`skip(condition)`**: This test *should ***never*** work* for `condition`
where `condition` is usually a certain browser like `FFOX` (for Firefox),
`WEBKIT` (for WebKit), and `CHROMIUM` (for Chromium).

For example, the [alt-click downloads test](https://github.com/microsoft/playwright/blob/471ccc72d3f0847caa36f629b394a028c7750d93/test/download.spec.js#L86) is marked
Copy link
Member Author

Choose a reason for hiding this comment

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

I think pointing to a concrete (permalinked) example is helpful, but this may throw people off about alt-click download once Playwright is fixed to support alt-click in WebKit and Chromium.

Consider adding a completely fiction example like:

it.fail(CHROMIUM)('should do exciting things', async({page, server} => {
     const excitingThings = await page.excitingThings()
     expect(excitingThings).toBeTruthy()
}))

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's a good point. On the other hand as long as it points to a particular revision of the file and not break over time I think it's ok. We can iterate on this later if it's unsatisfactory.

with `skip(FFOX)` since an alt-click in Firefox will not produce a download
even if a person was driving the browser.


- **`fail(condition)`**: This test *should ***eventually*** work* for `condition`
where `condition` is usually a certain browser like `FFOX` (for Firefox),
`WEBKIT` (for WebKit), and `CHROMIUM` (for Chromium).

For example, the [alt-click downloads test](https://github.com/microsoft/playwright/blob/471ccc72d3f0847caa36f629b394a028c7750d93/test/download.spec.js#L86) is marked
with `fail(CHROMIUM || WEBKIT)` since Playwright performing these actions
currently diverges from what a user would experience driving a Chromium or
WebKit.

## Public API Coverage

Every public API method or event should be called at least once in tests. To ensure this, there's a `coverage` command which tracks calls to public API and reports back if some methods/events were not called.
Expand Down