jupyter / nbclassic

Jupyter Notebook as a Jupyter Server extension
https://nbclassic.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
69 stars 60 forks source link

Fix ci end-to-end tests #274

Closed maartenbreddels closed 2 months ago

maartenbreddels commented 2 months ago

I think I have made the end to end tests green and a bit more stable.

I was hoping to get the CI fully green, but I am not sure why the downstream tests fail, but I can reproduce it locally by running on the jupyterlab_server main branch:

$ pytest tests/test_process.py tests/test_settings_api.py

It seems like the test_process_app in test_process.py in jupyterlab server makes all tests in test_settings_api in fail with:

self = <jupyter_server.extension.manager.ExtensionManager object at 0x17dd628b0>

    def sorted_extensions(self):
        """Dictionary with extension package names as keys
        and an ExtensionPackage objects as values.
        """
        # Sort the keys and
        keys = sorted(self.extensions.keys())
>       keys.remove("notebook_shim")
E       ValueError: list.remove(x): x not in list

keys       = []
self       = <jupyter_server.extension.manager.ExtensionManager object at 0x17dd628b0>

../../../miniconda3/envs/dev/lib/python3.9/site-packages/notebook_shim/nbserver.py:90: ValueError

Since this involves 3 packages (nbclassic+notebook_shim+jupyterlab_server) I am wondering if @blink1073 can take a look.

maartenbreddels commented 2 months ago

~~Strange, I've been running this on my fork, and the macos runs for the Playwright tests run fine (except 3.10): https://github.com/maartenbreddels/nbclassic/actions/runs/8835208298/job/24258782950~~

maartenbreddels commented 2 months ago

Pinning the OS'es should avoid breakage now and in the future.

It also seems playwright sometimes hangs on self.body.press('Escape') on osx in test test_save_readonly_as. We may want to skip that for just that OS, on Linux it seem pretty stable for now.

blink1073 commented 2 months ago

nbclassic is doing some gnarly things to configuration iirc, I think it makes sense to not test with the jupyterlab_server test suite.

maartenbreddels commented 2 months ago

@krassowski do you agree to take out that test?

krassowski commented 2 months ago

Sound good to me.

maartenbreddels commented 2 months ago

I also took out the pypy test, since the environment is unsolvable. This might get everything green/

maartenbreddels commented 2 months ago

I had the wrong label for macos, which made it not run.

I can rebase this last commit to squash this with the commit where I made this mistake to make history clean. That way we can merge (without squash) this commit. Do you have a preference?

krassowski commented 2 months ago

Over on other repos (lab/notebook) we just use squash merge, but if you prefer this to be commit-by-commit then we can merge with a merge commit.

Of note, maybe it is fine to only pin macos runner and keep others to -lateset. Not a strong preference though.

maartenbreddels commented 2 months ago

So 💚 I would strongly recommend pinning the OS to avoid unexpected CI breakage (like in the examples I put in the commit messages).

Let me do a final force push to polish this PR, and we get to see if we get flakyness.

maartenbreddels commented 2 months ago

I see an occasional failure in the end2end tests at my personal fork at https://github.com/maartenbreddels/nbclassic/actions but I think this PR is ready to be merged when it's also green here. Each commit is standalone, so I recommend a rebase+merge and not a squash.

maartenbreddels commented 2 months ago

💚