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

Update JS dependencies #1229

Merged
merged 4 commits into from
May 5, 2024
Merged

Update JS dependencies #1229

merged 4 commits into from
May 5, 2024

Conversation

mwouts
Copy link
Owner

@mwouts mwouts commented Apr 27, 2024

No description provided.

Copy link

github-actions bot commented Apr 27, 2024

Thank you for making this pull request.

Did you know? You can try it on Binder: Binder:lab or Binder:notebook.

Also, the version of Jupytext developed in this PR can be installed with pip:

HATCH_BUILD_HOOKS_ENABLE=true pip install git+https://github.com/mwouts/jupytext.git@update_js_dependencies

(this requires nodejs, see more at Developing Jupytext)

Copy link
Owner Author

@mwouts mwouts left a comment

Choose a reason for hiding this comment

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

please update playwright snapshots

Copy link
Owner Author

@mwouts mwouts left a comment

Choose a reason for hiding this comment

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

please update playwright snapshots

Copy link

codecov bot commented Apr 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.73%. Comparing base (c37f3e0) to head (e827858).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1229   +/-   ##
=======================================
  Coverage   97.73%   97.73%           
=======================================
  Files          29       29           
  Lines        4464     4464           
=======================================
  Hits         4363     4363           
  Misses        101      101           
Flag Coverage Δ
external 75.17% <ø> (ø)
functional 88.51% <ø> (ø)
integration 77.28% <ø> (ø)
unit 66.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mwouts mwouts changed the title Update JS dependencies Update JS dependencies and UI test screenshots Apr 28, 2024
@mwouts
Copy link
Owner Author

mwouts commented Apr 28, 2024

Hi @mahendrapaipuri , I am trying to update the UI tests as they fail on the CI, but I am not sure how to do this? Thanks!

@mahendrapaipuri
Copy link
Contributor

Why are you updating the screenshots? If you look into the new screenshots you are trying to update, there are kernels that are being picked from your local test env like itables, javascript, etc. But in our UI test env, we are just installing bash kernel to test the extension with a non-python kernel.

Normally, if you dont touch the extension code, you dont have to run the UI tests locally. If you really want to include more kernels in the test env (which I dont think it is necessary), you will need to update [test-ui] dependencies in pyproject.toml to install these kernels and then update screenshots.

Does it make sense?

@mwouts
Copy link
Owner Author

mwouts commented Apr 29, 2024

Hi @mahendrapaipuri , thanks for your comment! Indeed you're correct I don't want to add a new kernel on the CI. The reason why I am trying to update the UI tests is that they currently fail on the main branch (see e.g. this run).

As I was not able to trigger an update through please update playwright snapshots (see my attempts above) I tried to update them locally, but as you point out that comes with more differences. Do you think you could trigger a UI test update, maybe on another branch? Thanks!

@mahendrapaipuri
Copy link
Contributor

Hello @mwouts Understood. Some pointers that might be useful here:

  • Whenever UI tests fail, they create artifacts with screenshots and "diff" between expected and actual screenshots. For the run you pointed out on the main, you can find those at the bottom of CI Summary page. This allows us to inspect why they are failing. Maybe we can add this in docs for future references. On the question why they are failing, the screenshots in artifacts tells us there is 3 px difference between actual and expected screenshots and that is why they are failing. We have a tolerance set, but this can always be a annoying issue.
  • I think you made a good point here that we need to add a action to update the screenshots automatically as preparing local envs to generate correct snapshots can be tiring. Here is the action provided by jupyter maintainer tools. Issue Add CI workflow to update playwright snapshots automatically #1230 has been created

@mwouts
Copy link
Owner Author

mwouts commented Apr 30, 2024

Thanks @mahendrapaipuri ! Oh it's great that I can download the updated UI results, I will make a PR from this! Re the automation, thanks for the pointers too.

@mwouts mwouts added this to the 1.16.2 milestone May 1, 2024
@mwouts mwouts changed the title Update JS dependencies and UI test screenshots Update JS dependencies May 5, 2024
@mwouts mwouts mentioned this pull request May 5, 2024
@mwouts mwouts merged commit 7963f5f into main May 5, 2024
32 checks passed
@mwouts mwouts deleted the update_js_dependencies branch May 5, 2024 21:53
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.

2 participants