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

Switch from karma to web-test-runner #704

Merged
merged 12 commits into from
May 29, 2024

Conversation

fcollonval
Copy link
Member

@fcollonval fcollonval commented May 20, 2024

Fixes #610

Switch from karma to web-test-runner

I did not switch to jest to be able to test in browser directly and to not have to change the test (keep using mocha and chai).

Browsers are provided by playwright

@fcollonval fcollonval added the maintenance Dependencies, build, technical debt, etc. label May 20, 2024
@fcollonval
Copy link
Member Author

The nice consequence is to get the macOS webkit tests green again

Copy link
Contributor

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

Thanks for doing this maintenance work!

I left a few inline comments.

For some reason, I had to modify some tests in order for them to pass locally. Any idea what might be going on?

I was also wondering, would you mind leaving a note about how you created this pull request? Was it a lot of search/replace or were there some magic lerna commands that helped update the sub-package configs?

I'm requesting changes specifically only for one thing specifically: the missing docs for installing Playwright locally.

.github/workflows/tests.yml Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
@@ -18,15 +18,28 @@ yarn build:src

## Tests

The tests are written using karma to simulate a browser environment.
The tests are written using [web-test-runner](https://modern-web.dev/docs/test-runner/overview/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably in the section above we need to add a line for how to install the Playwright browser binaries, otherwise yarn test will not work.

We also need to specify which version of Yarn to use, because yarn playwright install worked for me in yarn v1 and v4 but not v3.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is weird that it does not work with v3.

Copy link
Contributor

@gabalafou gabalafou May 24, 2024

Choose a reason for hiding this comment

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

Yeah, I wonder if what I was seeing actually had more to do with my other comment (about putting @playwright/test in the top-level package.json)

examples/example-dockpanel-amd/package.json Show resolved Hide resolved
Co-authored-by: gabalafou <gabriel@fouasnon.com>
@fcollonval
Copy link
Member Author

Thanks for the review @gabalafou

The fact that you need to change the tests locally is surprising as they work for me locally and on the CI. Does it happens with all browsers?

What is your environment? NodeJS, OS?

Note for yarn, the real version used should be independent of your installed version as we include it with the repo: https://github.com/jupyterlab/lumino/tree/main/.yarn/releases

Config is there:

yarnPath: .yarn/releases/yarn-3.6.0.cjs

@fcollonval
Copy link
Member Author

I was also wondering, would you mind leaving a note about how you created this pull request? Was it a lot of search/replace or were there some magic lerna commands that helped update the sub-package configs?

Yeah lots of smart search/replace...

Copy link
Contributor

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

Testing this locally some more, I think we should yarn add --dev @playwright/test at the root level of the repo, and have users do yarn playwright install.

Reasons:

  • Correct me if I'm wrong, but at this point, Playwright is as much a dev dependency of the repo as TypeScript, so it seems like it should be there
  • If users just copy paste the yarn build:test and yarn test commands without reading the rest of the docs, they will get a message to run yarn playwright test, not npx playwright test.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@fcollonval fcollonval requested a review from gabalafou May 25, 2024 16:10
Copy link
Contributor

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

Okay, this runs "out of the box" now, thank you! (By which I mean, I can do a fresh install of the repo and run the tests without getting stuck along the way.)

I'm still getting the test failures I mentioned before. I checked that this doesn't happen before this PR so I think the failures are somehow related.

I'm running Node 20.2.0 and macOS 14.4.1.

What's really bizarre is if I run each browser separately--yarn test:webkit, yarn test:firefox, and yarn test:chromium--all the tests pass. It's only when I run yarn test that I get the 9 test failures.

Do you want to open an issue and punt on this for now, since the CI is fine?

Copy link
Contributor

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

I'm approving to unblock you. I'll let you decide how we want to address the tests failing on my machine :)

@fcollonval
Copy link
Member Author

Okay, this runs "out of the box" now, thank you! (By which I mean, I can do a fresh install of the repo and run the tests without getting stuck along the way.)

🎉

@fcollonval
Copy link
Member Author

I'm still getting the test failures I mentioned before. I checked that this doesn't happen before this PR so I think the failures are somehow related.
I'm running Node 20.2.0 and macOS 14.4.1.
Do you want to open an issue and punt on this for now, since the CI is fine?

If someone else could test it to see if you have an unlucky combination of software's or not. It would be nice.

Gentle ping to @JasonWeill, if you are on MacOS, would you have time to run the tests of this PR locally?

For reference, I'm on Debian 12 with Node.js 20.9.0.

@fcollonval
Copy link
Member Author

Thanks a lot @gabalafou for all the tests.

@JasonWeill
Copy link
Contributor

Running Lumino tests on tip-of-main pass, but on @fcollonval 's branch for this PR, they fail. I'm using a fresh Conda env on macOS Ventura 13.6.7.

Main branch
$ yarn test
lerna notice cli v7.1.4
lerna info versioning independent
lerna notice filter including "@lumino/!(example-)*"
lerna info filter [ '@lumino/!(example-)*' ]
 
    ✔  @lumino/properties:test (5s)
    ✔  @lumino/domutils:test (5s)
    ✔  @lumino/keyboard:test (5s)
    ✔  @lumino/algorithm:test (5s)
    ✔  @lumino/collections:test (3s)
    ✔  @lumino/coreutils:test (3s)
    ✔  @lumino/virtualdom:test (3s)
    ✔  @lumino/signaling:test (3s)
    ✔  @lumino/messaging:test (3s)
    ✔  @lumino/disposable:test (3s)
    ✔  @lumino/dragdrop:test (3s)
    ✔  @lumino/polling:test (7s)
    ✔  @lumino/commands:test (8s)
    ✔  @lumino/widgets:test (6s)
    ✔  @lumino/application:test (3s)
    ✔  @lumino/datagrid:test (3s)

 ————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————

 >  Lerna (powered by Nx)   Successfully ran target test for 16 projects (30s)
fcollonval's branch
$ yarn test
lerna notice cli v7.1.4
lerna info versioning independent
lerna notice filter including "@lumino/!(example-)*"
lerna info filter [ '@lumino/!(example-)*' ]
 
 >  Lerna (powered by Nx)   Running target test for 16 projects
    ✖  @lumino/algorithm:test
       > @lumino/algorithm@2.0.1 build:test
       > npm run clean:test && tsc --build tests && cd tests && rollup -c
       
       
       > @lumino/algorithm@2.0.1 clean:test
       > rimraf tests/lib tests/tsconfig.tsbuildinfo
       
       
       ./lib/index.spec.js → ./lib/bundle.test.js...
       (!) Circular dependencies
       ../../../node_modules/chai/lib/chai.js -> ../../../node_modules/chai/lib/chai/utils/index.js -> ../../../node_modules/chai/lib/chai/utils/addProperty.js -> ../../../node_modules/chai/lib/chai.js
       ../../../node_modules/chai/lib/chai.js -> ../../../node_modules/chai/lib/chai/utils/index.js -> ../../../node_modules/chai/lib/chai/utils/addMethod.js -> ../../../node_modules/chai/lib/chai.js
       ../../../node_modules/chai/lib/chai.js -> ../../../node_modules/chai/lib/chai/utils/index.js -> ../../../node_modules/chai/lib/chai/utils/overwriteProperty.js -> ../../../node_modules/chai/lib/chai.js
       ...and 3 more
       created ./lib/bundle.test.js in 369ms
       
       Chromium: |██████████████████████████████| 0/1 test files | 0 passed, 0 failed
       
       Running tests...
       
       Running 1 test files...
       
tests/lib/bundle.test.js:
       
        ❌ browserType.launch: Executable doesn't exist at /Users/jweill/Library/Caches/ms-playwright/chromium-1117/chrome-mac/Chromium.app/Contents/MacOS/Chromium
       ╔═════════════════════════════════════════════════════════════════════════╗
       ║ Looks like Playwright Test or Playwright was just installed or updated. ║
       ║ Please run the following command to download new browsers:              ║
       ║                                                                         ║
       ║     yarn playwright install                                             ║
       ║                                                                         ║
       ║ <3 Playwright Team                                                      ║
       ╚═════════════════════════════════════════════════════════════════════════╝ 
           browserType.launch: Executable doesn't exist at /Users/jweill/Library/Caches/ms-playwright/chromium-1117/chrome-mac/Chromium.app/Contents/MacOS/Chromium
           ╔═════════════════════════════════════════════════════════════════════════╗
           ║ Looks like Playwright Test or Playwright was just installed or updated. ║
           ║ Please run the following command to download new browsers:              ║
           ║                                                                         ║
           ║     yarn playwright install                                             ║
           ║                                                                         ║
           ║ <3 Playwright Team                                                      ║
           ╚═════════════════════════════════════════════════════════════════════════╝
               at /Users/jweill/git/lumino/node_modules/@web/test-runner-playwright/dist/PlaywrightLauncher.js:94:74
               at PlaywrightLauncher.getOrStartBrowser (/Users/jweill/git/lumino/node_modules/@web/test-runner-playwright/dist/PlaywrightLauncher.js:96:15)
               at PlaywrightLauncher.startSession (/Users/jweill/git/lumino/node_modules/@web/test-runner-playwright/dist/PlaywrightLauncher.js:41:36)
               at TestScheduler.startSession (/Users/jweill/git/lumino/node_modules/@web/test-runner-core/dist/runner/TestScheduler.js:103:70)
               at TestScheduler.runNextScheduled (/Users/jweill/git/lumino/node_modules/@web/test-runner-core/dist/runner/TestScheduler.js:83:30)
               at TestScheduler.schedule (/Users/jweill/git/lumino/node_modules/@web/test-runner-core/dist/runner/TestScheduler.js:47:14)
               at TestRunner.runTests (/Users/jweill/git/lumino/node_modules/@web/test-runner-core/dist/runner/TestRunner.js:92:28)
               at TestRunner.start (/Users/jweill/git/lumino/node_modules/@web/test-runner-core/dist/runner/TestRunner.js:65:22)
               at async startTestRunner (/Users/jweill/git/lumino/node_modules/@web/test-runner/dist/startTestRunner.js:46:9)
       
       Chromium: |██████████████████████████████| 1/1 test files | 0 passed, 0 failed
       
       Error while running tests.
       
       

 ————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————

 >  Lerna (powered by Nx)   Ran target test for 16 projects (4s)
 
    ✔    0/1 succeeded [0 read from cache]
 
    ✖    1/1 targets failed, including the following:
         - @lumino/algorithm:test

@krassowski
Copy link
Member

@JasonWeill did you run yarn playwright install as instructed by the updated CONTRIBUTING.md?

@JasonWeill
Copy link
Contributor

Sorry, I was looking at the old CONTRIBUTING.md file. After installing Playwright, then running the tests, I still see errors, but different errors.

yarn playwright install
$ yarn playwright install
Removing unused browser at /Users/jweill/Library/Caches/ms-playwright/chromium-1067
Removing unused browser at /Users/jweill/Library/Caches/ms-playwright/firefox-1408
Removing unused browser at /Users/jweill/Library/Caches/ms-playwright/webkit-1860
Downloading Chromium 125.0.6422.26 (playwright build v1117) from https://playwright.azureedge.net/builds/chromium/1117/chromium-mac-arm64.zip
134.6 MiB [====================] 100% 0.0s
Chromium 125.0.6422.26 (playwright build v1117) downloaded to /Users/jweill/Library/Caches/ms-playwright/chromium-1117
Downloading Firefox 125.0.1 (playwright build v1449) from https://playwright.azureedge.net/builds/firefox/1449/firefox-mac-13-arm64.zip
77.8 MiB [====================] 100% 0.0s
Firefox 125.0.1 (playwright build v1449) downloaded to /Users/jweill/Library/Caches/ms-playwright/firefox-1449
Downloading Webkit 17.4 (playwright build v2003) from https://playwright.azureedge.net/builds/webkit/2003/webkit-mac-13-arm64.zip
66.6 MiB [====================] 100% 0.0s
Webkit 17.4 (playwright build v2003) downloaded to /Users/jweill/Library/Caches/ms-playwright/webkit-2003
yarn test
$ yarn test
lerna notice cli v7.1.4
lerna info versioning independent
lerna notice filter including "@lumino/!(example-)*"
lerna info filter [ '@lumino/!(example-)*' ]
 
    ✔  @lumino/algorithm:test (8s)
    ✔  @lumino/coreutils:test (3s)
    ✔  @lumino/signaling:test (4s)
    ✔  @lumino/disposable:test (3s)
    ✔  @lumino/domutils:test (4s)
    ✔  @lumino/keyboard:test (3s)
    ✔  @lumino/virtualdom:test (4s)
    ✔  @lumino/dragdrop:test (4s)
    ✔  @lumino/commands:test (8s)
    ✔  @lumino/collections:test (3s)
    ✔  @lumino/messaging:test (4s)
    ✔  @lumino/properties:test (3s)

    ✖  @lumino/widgets:test
       > @lumino/widgets@2.3.2 build:test
       > npm run clean:test && tsc --build tests && cd tests && rollup -c
       
       
       > @lumino/widgets@2.3.2 clean:test
       > rimraf tests/lib tests/tsconfig.tsbuildinfo
       
       
       ./lib/index.spec.js → ./lib/bundle.test.js...
       (!) Circular dependencies
       ../../../node_modules/chai/lib/chai.js -> ../../../node_modules/chai/lib/chai/utils/index.js -> ../../../node_modules/chai/lib/chai/utils/addProperty.js -> ../../../node_modules/chai/lib/chai.js
       ../../../node_modules/chai/lib/chai.js -> ../../../node_modules/chai/lib/chai/utils/index.js -> ../../../node_modules/chai/lib/chai/utils/addMethod.js -> ../../../node_modules/chai/lib/chai.js
       ../../../node_modules/chai/lib/chai.js -> ../../../node_modules/chai/lib/chai/utils/index.js -> ../../../node_modules/chai/lib/chai/utils/overwriteProperty.js -> ../../../node_modules/chai/lib/chai.js
       ...and 3 more
       created ./lib/bundle.test.js in 817ms
       
       Chromium: |██████████████████████████████| 0/1 test files | 0 passed, 0 failed
       
       Running tests...
       
       Running 1 test files...
       
tests/lib/bundle.test.js:
       
        ❌ @lumino/widgets > Menu > #handleEvent() > mousemove > should set the active index
             AssertionError: expected -1 to equal +0
             + expected - actual
             
             --1
             +0
             
             at n.<anonymous> (tests/lib/bundle.test.js:39079:49)
       
        ❌ @lumino/widgets > Menu > #handleEvent() > mousemove > should open a child menu after a timeout
             AssertionError: expected -1 to equal +0
             + expected - actual
             
             --1
             +0
             
             at n.<anonymous> (tests/lib/bundle.test.js:39094:49)
       
        ❌ @lumino/widgets > Menu > #handleEvent() > mouseleave > should reset the active index
             AssertionError: expected -1 to equal +0
             + expected - actual
             
             --1
             +0
             
             at n.<anonymous> (tests/lib/bundle.test.js:39139:49)
       
        ❌ @lumino/widgets > Menu > #handleEvent() > mousedown > should not close the menu if on a child node
             AssertionError: expected false to equal true
             + expected - actual
             
             -false
             +true
             
             at n.<anonymous> (tests/lib/bundle.test.js:39160:48)
       
        ❌ @lumino/widgets > MenuBar > #handleEvent() > mousedown > should close an active menu
             AssertionError: expected -1 to equal +0
             + expected - actual
             
             --1
             +0
             
             at n.<anonymous> (tests/lib/bundle.test.js:40419:48)
       
        ❌ @lumino/widgets > MenuBar > #handleEvent() > mousedown > should open an active menu
             AssertionError: expected false to equal true
             + expected - actual
             
             -false
             +true
             
             at n.<anonymous> (tests/lib/bundle.test.js:40437:48)
       
        ❌ @lumino/widgets > MenuBar > #handleEvent() > mousedown > should not close an active menu if not a left mouse press
             AssertionError: expected -1 to equal +0
             + expected - actual
             
             --1
             +0
             
             at n.<anonymous> (tests/lib/bundle.test.js:40455:48)
       
        ❌ @lumino/widgets > MenuBar > #handleEvent() > mousedown > should not work on a menu bar item whose menu is empty
             AssertionError: expected +0 to equal -1
             + expected - actual
             
             -0
             +-1
             
             at n.<anonymous> (tests/lib/bundle.test.js:40472:48)
       
        ❌ @lumino/widgets > MenuBar > #handleEvent() > mousemove > should open a new menu if a menu is already open
             AssertionError: expected +0 to equal 1
             + expected - actual
             
             -0
             +1
             
             at n.<anonymous> (tests/lib/bundle.test.js:40487:48)
       
       Chromium: |██████████████████████████████| 1/1 test files | 926 passed, 9 failed, 11 skipped
       
       Finished running tests in 2.2s with 9 failed tests.
       
       

 ————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————

 >  Lerna (powered by Nx)   Ran target test for 16 projects (58s)
 
    ✔    12/13 succeeded [0 read from cache]
 
    ✖    1/13 targets failed, including the following:
         - @lumino/widgets:test

Co-authored-by: gabalafou <gabriel@fouasnon.com>
@fcollonval
Copy link
Member Author

Thanks for testing the PR @JasonWeill

As the errors are reproducible, I applied the fix proposed by @gabalafou in e96e2e3. Tests are passing for me locally - so 🤞 for the CI.

@fcollonval
Copy link
Member Author

Ok as the CI is green and the fix is working locally for @gabalafou

I'll merge this one.

@fcollonval fcollonval merged commit 6b92c7e into jupyterlab:main May 29, 2024
17 checks passed
@fcollonval fcollonval deleted the maintenance/karma-to-web-runner branch May 29, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Dependencies, build, technical debt, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Karma is deprecated we should move on
4 participants