Skip to content

upgrade e2e and enable firefox#206

Merged
stefcameron merged 1 commit intofocus-trap:masterfrom
liunate:e2e-upgrade-and-enable-firefox
Dec 8, 2020
Merged

upgrade e2e and enable firefox#206
stefcameron merged 1 commit intofocus-trap:masterfrom
liunate:e2e-upgrade-and-enable-firefox

Conversation

@liunate
Copy link
Copy Markdown
Contributor

@liunate liunate commented Dec 6, 2020

Features and Bug Fixes

- [ ] Issue being fixed is referenced.
- [ ] Unit test coverage added/updated.
- [ ] E2E test coverage added/updated.
- [ ] Prop-types added/updated.
- [ ] Typings added/updated.
- [ ] README updated (API changes, instructions, etc.).
- [ ] Changes to dependencies explained.
- [ ] Changeset added (run yarn changeset locally to add one, follow prompts).

Tooling

  • Issue being fixed is referenced.
  • Changes to dependencies explained.
    - [ ] README instructions updated.
  • A Changeset is not added.

DONE

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Dec 6, 2020

⚠️ No Changeset found

Latest commit: f4a845b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@liunate liunate force-pushed the e2e-upgrade-and-enable-firefox branch from 1827905 to f4a845b Compare December 6, 2020 16:02
browser: [chrome, firefox]
container:
image: cypress/included:6.0.1
options: --user 1001
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Per official cypress git action example for Firefox: https://github.com/cypress-io/github-action#firefox

Comment on lines +64 to +68
- uses: actions/checkout@v1
- uses: cypress-io/github-action@v2.6.1
with:
start: yarn start
browser: ${{ matrix.browser }}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the suggested git action way to run cypress test on CI by bamutov

@liunate
Copy link
Copy Markdown
Contributor Author

liunate commented Dec 6, 2020

One comment out of this change but I just thinked about at the trigger condition of e2e test https://github.com/focus-trap/focus-trap-react/pull/206/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR6

on:
  push:
    branches:
      - 'master'

Do we benefit from running e2e at every commit push so submitter can find out real browser issue much earlier than PR creation? I feel this repo e2e is really fast.

@liunate
Copy link
Copy Markdown
Contributor Author

liunate commented Dec 6, 2020

If the change is good I will also port this to the base repo focus-trap

Copy link
Copy Markdown
Member

@stefcameron stefcameron left a comment

Choose a reason for hiding this comment

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

Fantastic! Thanks for keeping track of the Cypress FF issue. 🎉

@stefcameron
Copy link
Copy Markdown
Member

Do we benefit from running e2e at every commit push so submitter can find out real browser issue much earlier than PR creation? I feel this repo e2e is really fast.

AFAIK, the CI action is configured such that it will run for any PR update (whether you force-push the branch or add another commit) as well as on master for all merges/commits to master.

Are you seeing something different?

@stefcameron
Copy link
Copy Markdown
Member

If the change is good I will also port this to the base repo focus-trap

Yes, please do!

@stefcameron stefcameron merged commit 2375941 into focus-trap:master Dec 8, 2020
@stefcameron stefcameron mentioned this pull request Dec 8, 2020
5 tasks
@stefcameron
Copy link
Copy Markdown
Member

FYI, I have now made the FF e2e tests in CI mandatory, as in must pass before merging.

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.

2 participants