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 ci end-to-end tests #274

Merged
merged 8 commits into from
Apr 26, 2024
Merged

Fix ci end-to-end tests #274

merged 8 commits into from
Apr 26, 2024

Conversation

maartenbreddels
Copy link
Contributor

No description provided.

@maartenbreddels
Copy link
Contributor Author

maartenbreddels commented Apr 25, 2024

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
Copy link
Contributor Author

maartenbreddels commented Apr 25, 2024

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
Copy link
Contributor Author

maartenbreddels commented Apr 25, 2024

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
Copy link
Contributor

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

@maartenbreddels
Copy link
Contributor Author

@krassowski do you agree to take out that test?

@krassowski
Copy link
Member

Sound good to me.

@maartenbreddels
Copy link
Contributor Author

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

@maartenbreddels
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

💚

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thank you!

@krassowski krassowski merged commit 5c85fee into jupyter:main Apr 26, 2024
32 checks passed
krassowski pushed a commit that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants