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

Fix circular dependencies that pollutes version bump #12495

Merged

Conversation

fcollonval
Copy link
Member

@fcollonval fcollonval commented Apr 29, 2022

References

Fix #11630
Fix #5821

We have 2 circular dependencies to break. The major trouble being testutils. We cannot depend on test utils for all packages. But as it is published, we probably needs to keep publishing it although the best approach would be to internalize the utils in each package => This is done by adding a src/testutils.ts that is not exposed through the src/index.ts.

2022-04-29T13:17:46.6049359Z lerna WARN ECYCLE @jupyterlab/application -> @jupyterlab/rendermime -> @jupyterlab/markedparser-extension -> @jupyterlab/application
2022-04-29T13:17:46.6050993Z lerna WARN ECYCLE @jupyterlab/apputils -> @jupyterlab/testutils -> @jupyterlab/notebook -> @jupyterlab/cells -> @jupyterlab/attachments -> (nested cycle: @jupyterlab/application -> @jupyterlab/rendermime -> @jupyterlab/markedparser-extension -> @jupyterlab/application) -> @jupyterlab/apputils

lerna does not ignore circular dependencies that come from dev dependencies. So when bumping the version it is spreading to far more packages than needed.

Code changes

Create a new package @jupyterlab/testing that contains the configuration for babel and jest as well as all helpers for testing @jupyterlab/services.

@jupyterlab/coreutils is the only package not using that new package for its tests.

The helpers in @jupyterlab/testutils have been split between the packages in a testutils.ts file (or folder) that is not directly exported by their index.

Some tests have chicken/egg trouble (packages codeeditor, completer and rendermime are using their downstream packages for unit tests 😞) -> the quick solution done here where to move those test files in metapackage.

  • TBC The package @jupyterlab/testutils exports all previous helpers. But they main not be gathered under the same namespace.

User-facing changes

N/A

Backwards-incompatible changes

Hopefully few

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@fcollonval
Copy link
Member Author

Current top circular loop detected by lerna:

lerna WARN ECYCLE @jupyterlab/apputils -> @jupyterlab/services -> @jupyterlab/nbformat -> @jupyterlab/testutils -> @jupyterlab/notebook -> @jupyterlab/cells -> @jupyterlab/apputils

@fcollonval
Copy link
Member Author

Ok indeed this fixes it. Big thanks to @blink1073 🥇 🚀

@jtpio
Copy link
Member

jtpio commented Dec 22, 2022

This is done by adding a src/testutils.ts that is not exposed through the src/index.ts.

This might look a bit odd and unusual at first since this was not really a pattern used across the lab code base until now. But yeah I am also not sure whether there would be another way to fix the circular dependency without doing something like this.

@fcollonval fcollonval force-pushed the fix/11630-testutils-cycle-dep branch from da601a0 to ece7937 Compare January 3, 2023 11:04
@fcollonval fcollonval merged commit 1d36ce5 into jupyterlab:master Jan 3, 2023
@fcollonval fcollonval deleted the fix/11630-testutils-cycle-dep branch January 3, 2023 14:02
open: () => {
/* noop */
}
} as any,
Copy link
Member

Choose a reason for hiding this comment

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

What was the reason for not using the DocumentWidgetOpenerMock here?

Copy link
Member Author

Choose a reason for hiding this comment

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

A miss in the many rebase

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll push a follow-up PR

hbcarlos pushed a commit to hbcarlos/jupyterlab that referenced this pull request Jan 29, 2023
* Create @jupyterlab/testing

Move tests from codeeditor to codemirror
This is needed to respect package hierarchy

Refactor test to have logical dependencies graph

* Add all test dependencies for coreutils

* Internalize retry setup and server start up

* Remove testutils from new files

* Restore the testutils API as much as possible

* Fix tests

* Fix debugger tests

* Update timeout for flaky test

* Fix setup/teardown

* Advice to use `runInBand` when running locally

* Fix running all tests at once locally

* [skip ci] Update docs/source/extension/extension_migration.rst

Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>

* Increase timeout for last timed out test

Locally all tests are passing (thanks to the --runInBand option)
Hopefully this one can pass on the CI too.

* Automatic application of license header

* Undo custom timeout setting

* Apply changes to @jupyterlab/metadataform

Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
hbcarlos pushed a commit to hbcarlos/jupyterlab that referenced this pull request Jan 29, 2023
* Create @jupyterlab/testing

Move tests from codeeditor to codemirror
This is needed to respect package hierarchy

Refactor test to have logical dependencies graph

* Add all test dependencies for coreutils

* Internalize retry setup and server start up

* Remove testutils from new files

* Restore the testutils API as much as possible

* Fix tests

* Fix debugger tests

* Update timeout for flaky test

* Fix setup/teardown

* Advice to use `runInBand` when running locally

* Fix running all tests at once locally

* [skip ci] Update docs/source/extension/extension_migration.rst

Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>

* Increase timeout for last timed out test

Locally all tests are passing (thanks to the --runInBand option)
Hopefully this one can pass on the CI too.

* Automatic application of license header

* Undo custom timeout setting

* Apply changes to @jupyterlab/metadataform

Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.