jupytext icon indicating copy to clipboard operation
jupytext copied to clipboard

Update JS dependencies

Open mwouts opened this issue 1 year ago • 7 comments

mwouts avatar Apr 27 '24 22:04 mwouts

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)

github-actions[bot] avatar Apr 27 '24 22:04 github-actions[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 27 '24 22:04 codecov[bot]

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!

mwouts avatar Apr 28 '24 10:04 mwouts

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?

mahendrapaipuri avatar Apr 29 '24 08:04 mahendrapaipuri

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!

mwouts avatar Apr 29 '24 19:04 mwouts

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 #1230 has been created

mahendrapaipuri avatar Apr 30 '24 09:04 mahendrapaipuri

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 avatar Apr 30 '24 21:04 mwouts