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

Jump to bottom

Improve cross browser test coverage: enable WebKit tests on CI #990

Closed
2 tasks done
jattasNI opened this issue Jan 23, 2023 · 4 comments · Fixed by #2237
Closed
2 tasks done

Improve cross browser test coverage: enable WebKit tests on CI #990

jattasNI opened this issue Jan 23, 2023 · 4 comments · Fixed by #2237
Assignees
@m-akinc
Labels
tech debt

Comments

@jattasNI
Copy link
Contributor

jattasNI commented Jan 23, 2023 •
edited by m-akinc
Loading

🧹 Tech Debt

We currently run unit tests in only Chromium and our Chromatic tests in Chromium and Firefox. More test coverage would be better to help catch issues like #984 via automated rather than manual testing.

Possible directions (might split this issue to track these separately)

  1. add commands to run unit tests on other browsers and run them locally (I tried this briefly and discovered several failing in Safari and Firefox) Done

  2. run tests as part of our PR build. Would require some research to find out best practices for getting agents with Firefox and webkit Done for Firefox

    • mac agents? Probably not as they cost many times more than default Linux agents
    • Using Playwright's cross-platform webkit build? Related: Set up Blazor end-to-end tests #705
    • we'd also want a way to disable specific tests in specific browsers. We could consider the approach we use for WebVIs where certain tags are added to test names. Done

  3. Consider whether we want to run cross-browser testing for nimble-tokens, nimble-angular, and nimble-blazor

Already done by @fredvisser :

  • Once it's available, enable Chromatic testing in Safari (at a recent meeting we heard it was coming in February)

Related PRs
Beta Give feedback
  1. Fix table row rendering in Safari #998
  2. Unit test commands for WebKit, Playwright provides browser binaries for tests #1067
@jattasNI jattasNI added tech debt triage New issue that needs to be reviewed labels Jan 23, 2023
@m-akinc m-akinc removed the triage New issue that needs to be reviewed label Jan 24, 2023
@jattasNI jattasNI mentioned this issue Jan 26, 2023
Merged
1 task
jattasNI added a commit that referenced this issue Jan 26, 2023
@jattasNI
Fix table row rendering in Safari (#998)
2e5a1c8 
# Pull Request

## 🤨 Rationale

Fixes #984 

## 👩‍💻 Implementation

As @rajsite and @msmithNI suggested in the linked issue, use
`this.$fastController.isConnected` instead of `this.isConnected` in the
table cell's `cellTemplateChanged()` method.

## 🧪 Testing

I manually tested in Safari in Storybook and the Angular example app.
Malcolm tested in Playwright's Safari.

Auto tests would be covered by #990.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@msmithNI msmithNI mentioned this issue Feb 18, 2023
Merged
1 task
@msmithNI
Copy link
Contributor

msmithNI commented Feb 21, 2023 •
edited
Loading

Created #1074 and #1075 to track our current autotest failures in WebKit and Firefox.

msmithNI added a commit that referenced this issue Feb 23, 2023
@msmithNI
Unit test commands for WebKit, Playwright provides browser binaries f…
b3a6c58 
…or tests (#1067)

# Pull Request

## 🤨 Rationale

Partially addresses #990 

Since we've recently uncovered several Safari/WebKit-specific bugs, we
want to make it easier for developers to catch issues in Safari
(especially if they only have Windows machines).

## 👩‍💻 Implementation

- Add `playwright` as dev dependency
- Uptake Playwright for providing Chromium/Firefox/WebKit binaries for
the test runs
- Remove `puppeteer` dependency (was used for Chromium binaries)
-  Add `karma-webkit-launcher` as dev dependencies to nimble-components
-
[karma-webkit-launcher](https://github.com/google/karma-webkit-launcher)
uses Playwright to launch a WebKit-based browser for tests.
- Playwright-WebKit is always used if Playwright is available; we can
add a separate command for Safari/Mac test running if we want to.
- Add commands `test-webkit`, `test-webkit:debugger` and
`storybook-open-webkit` to nimble-components. These will use the
Playwright-provided WebKit binaries.
- Update docs to mention new test commands.

## 🧪 Testing

- Ran commands locally (note: we have some pre-existing WebKit and
Firefox test failures)
- PR/CI build. Note that this PR will not enable the tests to run
automatically against WebKit on our PR/CI builds. When I tried that, I
ran into issues with the `npm run test` stage of the build hanging
(though it looked like the WebKit tests had already completed), so
enabling the tests there will need some more investigation. We also have
some pre-existing test failures on both Firefox and WebKit/Safari that
we need to address in some manner.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
@m-akinc m-akinc moved this from In progress to Defined/Ready to Pickup in Nimble Design System Priorities Mar 6, 2023
@msmithNI
Copy link
Contributor

msmithNI commented Apr 4, 2023

@jattasNI jattasNI mentioned this issue Apr 6, 2023
Merged
1 task
@rajsite rajsite mentioned this issue Jan 12, 2024
Open
@jattasNI
Copy link
Contributor Author

jattasNI commented Mar 4, 2024

@msmithNI looks like we are now running tests on the CI for Firefox and skipping ones that are tracked by #1075. Can you confirm that the next step for this tech debt would be to file an issue or discussion with karma-webkit-launcher?

@jattasNI jattasNI moved this from Defined/Ready to Pickup to Current Iteration in Nimble Design System Priorities Mar 4, 2024
@msmithNI
Copy link
Contributor

msmithNI commented Mar 7, 2024

@jattasNI that's mostly accurate.

Initially I thought that it would be a quick fix for karma-webkit-launcher but I'm not sure about that anymore. A few months ago I tried essentially forking karma-webkit-launcher (copied the code to a local source file/ Karma plugin) to test out the process cleanup code that was already there, but I didn't get it working. (That was this branch / this build result)

So options are probably

  • File an issue/discusson with karma-webkit-launcher: Optimally we could point them to a smaller reproducing repo than Nimble (since we have multiple projects/ test runs all using the same browser Playwright binaries)
  • Potentially fork the launcher / make our own version (not a ton of code, we'd probably end up with less than karma-webkit-launcher since it also supports non-Playwright test runs)

@jattasNI jattasNI changed the title Improve cross browser test coverage Improve cross browser test coverage: enable WebKit tests on CI Mar 27, 2024
@m-akinc m-akinc moved this from Current Iteration to In progress in Nimble Design System Priorities Jun 7, 2024
@m-akinc m-akinc self-assigned this Jun 7, 2024
@m-akinc m-akinc mentioned this issue Jul 2, 2024
Merged
1 task
@m-akinc m-akinc moved this from In Progress to Done in Nimble Design System Priorities Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@m-akinc m-akinc

Labels
tech debt
Projects
Nimble Design System Priorities
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Run webkit tests on pipeline builds ni/nimble
3 participants
@m-akinc @jattasNI @msmithNI