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
Remove Selenium and Karma #13401
Remove Selenium and Karma #13401
Conversation
Note: a huge help was [this](8986c45) unmerged commit by Tom Robinson.
Codecov Report
@@ Coverage Diff @@
## master #13401 +/- ##
=======================================
Coverage 83.05% 83.05%
=======================================
Files 326 326
Lines 25968 25968
Branches 1892 1891 -1
=======================================
Hits 21567 21567
- Misses 2509 2510 +1
+ Partials 1892 1891 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I love that diff stat +43 −871
|
||
// NOTE (based on original Tom Robinson's note from 1/7/2019): | ||
// this worked in karma but jsdom doesn't have the required APIs on div/contenteditable | ||
xit("should get/set selection on contenteditable correctly", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe these tests can be deleted rather than skipped? Our Cypress test of custom expressions somewhat covers this.
In general, I'd say this unit-level coverage is good, but given the browser APIs required, I think relying on Cypress is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaic, even better :)
Very excited about this. 🥳 |
@dacort How is this still waiting on ci/circleci |
(blocking the merge of #13401)
Not sure - maybe the checks below are still using the config from the target branch? It looks like in your individual commits, the |
Can we force merge, if all other tests pass? |
Yep, just need an admin to do it. |
That would be... @camsaul ? All tests finally passed. Please squash and merge this into master. Thanks 🙏 |
Status
READY
What does this PR accomplish?
frontend/test/legacy-karma/lib/dom.spec.js
into a unit testfrontend/test/metabase/lib/dom.unit.spec.js
andskipsremoves 2 tests in that group (lack of support for testing selection API in div/contenteditable)How to test this manually?
yarn test-unit
(all tests should pass)alternatively
yarn test-unit-watch dom
(runs the single specfrontend/test/metabase/lib/dom.unit.spec.js
)2 should be skippedAdditional context: