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

test(swc): trying to verify swc_core with local pkg #5452

Closed
wants to merge 2 commits into from

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Aug 11, 2022

Description:

This PR attempts to add another set of e2e test to ensure missing version bump in some transitive deps, which are not able to be caught in current CI as it uses path resolutions all the time.

Idea's straightforward, for the pkgs we'd like to ensure (swc_core), make a compile test to build against locally packed (cargo package) instead of path resolution. generated pkg will try to behave as close to actually published one without path resolution, so we can catch some of build time errors before publish.

Depends on usefulness plan to grow test more, especially some regression tests between @swc/core to plugin for the newly published pkg. This is bit headache with existing plugins (swc-coverage-instrument) and having test will help future regressions.

BREAKING CHANGE:

Related issue (if exists):

@kdy1 kdy1 added this to the Planned milestone Aug 11, 2022
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

I think it should not be a js test.
It will cause headaches while publishing

@kwonoj
Copy link
Member Author

kwonoj commented Aug 12, 2022

It will cause headaches while publishing

Will it? theoritically this'll ensure test catches in PR level.

@kdy1
Copy link
Member

kdy1 commented Aug 12, 2022

Yep, you can use ci branch to verify it. I was working on ci fix, but feel free to override it

@kwonoj
Copy link
Member Author

kwonoj commented Aug 12, 2022

Before digging further, would like to hear workflow which'll break publishing.

The main goal of this PR is (if it works correctly)

  • in each PR / or CI run, verify against soon-to-be-published pkg to verify compilation
  • if bulid fails, we know some pkg bump is missing.

If using plain js is not right place, where would be a good place? I don't think where / how to execute it changes the way it works in this PR but I may wrong.

@kdy1
Copy link
Member

kdy1 commented Aug 12, 2022

We run all js tests before publishing the node package.
It should be done using shell script, not jest-based testing system

@kwonoj
Copy link
Member Author

kwonoj commented Aug 12, 2022

publishing the node package.

But test doesn't do anything with node.js package, am I missing something?

@kdy1
Copy link
Member

kdy1 commented Aug 12, 2022

No, test doesn't do anything with node.js package is the exact reason we don't need to run this test in all platforms

@kwonoj
Copy link
Member Author

kwonoj commented Aug 12, 2022

No, test doesn't do anything with node.js package is the exact reason we don't need to run this test in all platforms

I think there's some lacks of explanation in this PR. After this PR, test will be expanded

  • build native / wasm plugin using prepublished rust pkg (verify compilation)
  • use locally built latest node.js binding (not published version), run it with above plugin to verify runtime behavior matches after compilation success.

latter definitely needs node.js environment, the only thing we do not do is explicitly will-be-published node.js binding version install which'll obviously fail.

@kdy1
Copy link
Member

kdy1 commented Aug 12, 2022

We don't need all platform for such task. Using linux is enough

@kdy1
Copy link
Member

kdy1 commented Aug 12, 2022

If you really want to use js testing system, please create another PR using ci branch.

@kwonoj
Copy link
Member Author

kwonoj commented Aug 12, 2022

We don't need all platform for such task. Using linux is enough

I would argue #5406 is counterproof. Currently this only happens on certain machine type, in result linux CI / windows CI never caught this.

@kdy1
Copy link
Member

kdy1 commented Aug 12, 2022

Also, when creating another PR, please expand platform matrix of node js integration test so we can catch issues while normal review process

@kwonoj
Copy link
Member Author

kwonoj commented Aug 17, 2022

We're now using swc_core public version, so we can check this when we build binding_*.

@kwonoj kwonoj closed this Aug 17, 2022
@kwonoj kwonoj deleted the test-local-packed branch August 17, 2022 06:33
@kdy1 kdy1 modified the milestones: Planned, v1.2.238, v1.2.239 Aug 17, 2022
@swc-project swc-project locked as resolved and limited conversation to collaborators Oct 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants