JuliaGizmos / WebIO.jl

A bridge between Julia and the Web.
https://juliagizmos.github.io/WebIO.jl/latest/
Other
228 stars 64 forks source link

Fix frontend test #501

Closed olegshtch closed 1 year ago

olegshtch commented 1 year ago

Updates webpack to fix work on new nodejs.

codecov[bot] commented 1 year ago

Codecov Report

Merging #501 (7ac7bd9) into master (abe666d) will decrease coverage by 0.42%. The diff coverage is 100.00%.

:exclamation: Current head 7ac7bd9 differs from pull request most recent head eff1f90. Consider uploading reports for the commit eff1f90 to get more accurate results

@@            Coverage Diff             @@
##           master     #501      +/-   ##
==========================================
- Coverage   59.84%   59.41%   -0.43%     
==========================================
  Files          16       16              
  Lines         757      754       -3     
==========================================
- Hits          453      448       -5     
- Misses        304      306       +2     
Impacted Files Coverage Δ
src/scope.jl 66.93% <100.00%> (+1.58%) :arrow_up:
src/WebIO.jl 68.00% <0.00%> (-12.00%) :arrow_down:
src/messaging.jl 19.75% <0.00%> (-2.47%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

mind6 commented 1 year ago

So this should now be merged with #500?

olegshtch commented 1 year ago

I suppose this PR should be merged before to get clean CI.

MasonProtter commented 1 year ago

These changes aren't compatible with Observables v0.4, so we need to drop compatibility with it and only support v0.5. Also, Observables.listeners is used in two additional places here that need updating. I've proposed these changes as a PR to your PR branch here: https://github.com/olegshtch/WebIO.jl/pull/1

halleysfifthinc commented 1 year ago

This is great! I think we should probably keep the various PR's separate as much as possible. Are the changes for Observable listeners necessary to get tests passing again?

I'm not very familiar with javascript, but the changes look reasonable. Changing the minimum tested Julia version to 1.6 LTS LGTM, but we should also change the julia compat entry to 1.6 as well (or drop 0.7 at the very least).

We might also want to hold off on the WebIO version bump given other current PR's that could form a more substantial minor version release.

olegshtch commented 1 year ago

@halleysfifthinc No, Observable listeners don't related to tests. Sounds reasonable, I've removed additional commits.

MasonProtter commented 1 year ago

Those commits about listeners are required to fix InteractBase.jl for me. Is the idea here to do those changes in #497? What's wrong with just doing them together?

halleysfifthinc commented 1 year ago

My thought was to keep PRs separate and as small as possible, which is often preferred by maintainers (but I don't know for WebIO maintainers). I'm equally invested in getting #497 merged (along with #498) to fix PlotlyJS plot updates in Jupyter.

halleysfifthinc commented 1 year ago

In case there was any confusion, my suggestion was to reduce the scope of this PR to solely fix the frontend tests, and remove both mine and @MasonProtter s contributions for fixing #497 from this PR. Multiple currently open PRs (#500, #497, #498) would benefit from having the master branch passing tests again.

The reduced scope could be a quicker/easier merge.

olegshtch commented 1 year ago

Removed all commits not related to frontend.

fonsp commented 1 year ago

Does someone understand the test failures? Is it related to this PR?

olegshtch commented 1 year ago

@fonsp Yes, this PR fixes test failure in bundling.jl

halleysfifthinc commented 1 year ago

The remaining test failures are unrelated to bundling and are solved by #497.

MasonProtter commented 1 year ago

Seems really weird to me to split up the fixes into two separate PRs, since now we have two PRs where the tests don't pass rather than one PR where all tests pass.

MasonProtter commented 1 year ago

Regardless, @pfitzseb, @travigd, @shashi or @timholy could someone merge this? This and #497 are ready and if taken together get all the tests passing

MasonProtter commented 1 year ago

Thanks @pfitzseb, would you mind triggering the JuliaRegistrator? @fonsp added me to the repo but I'm not allowed to commit or trigger the registrator.