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 frontend test #501

Merged
merged 1 commit into from
Nov 9, 2022
Merged

Conversation

olegshtch
Copy link
Contributor

@olegshtch olegshtch commented Nov 2, 2022

Updates webpack to fix work on new nodejs.

@codecov
Copy link

codecov bot commented Nov 2, 2022

Codecov Report

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

❗ 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%) ⬆️
src/WebIO.jl 68.00% <0.00%> (-12.00%) ⬇️
src/messaging.jl 19.75% <0.00%> (-2.47%) ⬇️

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

@mind6
Copy link

mind6 commented Nov 3, 2022

So this should now be merged with #500?

@olegshtch
Copy link
Contributor Author

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

@MasonProtter
Copy link
Collaborator

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: olegshtch#1

@halleysfifthinc
Copy link
Contributor

halleysfifthinc commented Nov 5, 2022

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

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

@MasonProtter
Copy link
Collaborator

MasonProtter commented Nov 5, 2022

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

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

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

Removed all commits not related to frontend.

@fonsp
Copy link
Contributor

fonsp commented Nov 8, 2022

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

@olegshtch
Copy link
Contributor Author

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

@halleysfifthinc
Copy link
Contributor

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

@MasonProtter
Copy link
Collaborator

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

MasonProtter commented Nov 9, 2022

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

@pfitzseb pfitzseb merged commit 2614f4d into JuliaGizmos:master Nov 9, 2022
@olegshtch olegshtch deleted the fix-frontend-test branch November 9, 2022 11:10
@MasonProtter
Copy link
Collaborator

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.

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.

6 participants