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

modernize browser_check: refactor chrome-test.js -> browser-test.js as per jupyterlab core #106

Merged
merged 5 commits into from
May 26, 2021

Conversation

telamonian
Copy link
Contributor

Update the browser_check CI to match recent changes made to jupyterlab.browser_check. Hopefully this will also make that CI less flaky

@github-actions
Copy link

Binder Launch a binder notebook on branch telamonian/jupyter-fs/modernize-browser-check

…twice

- I basically just had to twiddle the traitlets until the import, calling, and config syntax/semantics acheived the holy alignment. Took me 6 hours

- I still don't fully undertand the root cause of BrowserApp being run twice w.r.t. the underlying libs (ie `jupyter_server` and maybe also `traitlets`), but I do understand why it was causing stochastic CI failures, and I also undertand the proximal cause of the bug in this project's own codebase

- Essentially, `BrowserApp.launch_instance()` was not only getting called twice, but getting called twice in instant succession; according to the log timestamps, the two `.launch_instance` calls were only about ~.005 s apart. This caused frequent crashes of the `yarn add playwright` cmd that `BrowserApp` launches as a subprocess, and could apparently also cause
issues during the browser_check run itself. Possibility: the browser_check bugs may have been caused by clashes between the two identical playwright instances trying to lock the exact same resources on the exact same browser at nearly the exact same time

- I'll start by opening a issue in `jupyter_server` and then maybe also `traitlets`
@telamonian telamonian force-pushed the modernize-browser-check branch from ce4b117 to 6647358 Compare May 26, 2021 03:10
@telamonian telamonian force-pushed the modernize-browser-check branch from 1d0a184 to 605e121 Compare May 26, 2021 04:03
- too heavy, don't actually need while we have just the one typescript package

- waaaay too big, waaaay too many subdeps; by itself, it added **82 Mb** to `node_modules` along with hundreds of new packages via its bezoar of dependencies
@telamonian telamonian force-pushed the modernize-browser-check branch from 605e121 to 33178d9 Compare May 26, 2021 04:04
@telamonian telamonian merged commit fb3738e into jpmorganchase:main May 26, 2021
@telamonian
Copy link
Contributor Author

Okay, that was genuinely awful. But its fixed now

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.

1 participant