Add a new decorator @requires_npm_package('package-name') #24430

juj · 2025-05-29T12:11:37Z

Add a new decorator @requires_npm_package('package-name') to allow disabling tests that require on a specific NPM development package. This allows running the test suite with production-only packages.

…sabling tests that require on a specific NPM development package. This allows running the test suite with production-only packages.

sbc100 · 2025-05-29T13:37:36Z

Why would we want to run tests without dev dependencies?

The packaged version of emscripten probably shouldn't even include the test suite, right? Where-as the developer should always have dev dependencies installer (i.e. via bootstrap).

juj · 2025-05-29T13:54:04Z

Why would we want to run tests without dev dependencies?

To verify that Emscripten works for users who have deployed only the release dependencies. Otherwise we cannot verify that what we ship is something that will work for end users?

sbc100 · 2025-05-29T15:02:42Z

I'm not sure its worth adding yet another test mode for this. Unless you have evidence that this has actually happened or is likely to happen.

juj · 2025-05-29T15:19:37Z

I'm not sure its worth adding yet another test mode for this. Unless you have evidence that this has actually happened or is likely to happen.

How do you mean "yet another test mode"? Isn't the decorator more like an advertisement/acknowledgement of "this test depends on that feature" - that seems like a convenient way to mark these types of dependencies.

Without this PR, if I do

git clone https://github.com/emscripten-core/emsdk.git
cd emsdk
emsdk install sdk-main-64bit
emsdk activate sdk-main-64bit
cd emscripten\main
test\runner core0

then test_source_map* will fail in the test suite.

So currently that gives me no good way of verifying that the Emscripten that I package for Unity users passes all tests, save for starting to manually scribble up notes about which tests are expected to fail, or write a script that allows some failures. That would feel janky.

End users won't have the development packages, so I think it would be good to be able to have the test suite reflect on that internally to be able to skip tests that won't be expected to work for end users? That way one can run the test suite in the exact configuration that end users have and it'll verify that no new dev package dependencies were accidentally introduced in Emscripten development?

sbc100 · 2025-05-29T16:55:57Z

I'm not sure its worth adding yet another test mode for this. Unless you have evidence that this has actually happened or is likely to happen.

How do you mean "yet another test mode"? Isn't the decorator more like an advertisement/acknowledgement of "this test depends on that feature" - that seems like a convenient way to mark these types of dependencies.

What I mean is now we have to have another test configuration called something like core0-no-deps that verifies that some subset of the test suite is runningable without dev dependencies.

Without this PR, if I do
git clone https://github.com/emscripten-core/emsdk.git
cd emsdk
emsdk install sdk-main-64bit
emsdk activate sdk-main-64bit
cd emscripten\main
test\runner core0
then test_source_map* will fail in the test suite.

I think its easy enough to simply add npm ci before you do ./test/runner isn't it?

I think you are the only person doing the above workflow by the way. As far as I know everyone else who runs the test suite is doing so from a git checkout and running ./bootstrap first (which installs the dev dependencies).

So currently that gives me no good way of verifying that the Emscripten that I package for Unity users passes all tests, save for starting to manually scribble up notes about which tests are expected to fail, or write a script that allows some failures. That would feel janky.

End users won't have the development packages, so I think it would be good to be able to have the test suite reflect on that internally to be able to skip tests that won't be expected to work for end users? That way one can run the test suite in the exact configuration that end users have and it'll verify that no new dev package dependencies were accidentally introduced in Emscripten development?

I don't think we should make it a goal for end users to be able to run the test suite, at least not by default. I don't even think we should be shipping the test suite to end users by default.

I'm inclined to say that if you want to run the test suite you need to checkout emscripten using git and run ./bootstrap first.

sbc100 · 2025-05-29T16:50:38Z

test/common.py

+  # Cache which packages have been resolved to avoid individual tests all re-running the same 'npm list' command.
+  resolved_packages = None
+
+  def require_npm_package(self, package):


If we do land this can we keep in simple.

No need to run npm maybe? Just a new EMTEST_SKIP_NODE_DEV_DEPENDENCIES environment variable that skips a certain set of tests.

I'd be OK with landing this much simpler version of this PR.

Thanks - ok I'll refactor into that form.

juj · 2025-05-29T17:01:27Z

everyone else who runs the test suite is doing so from a git checkout and running ./bootstrap first (which installs the dev dependencies).

Running bootstrap does not install dev dependencies if one has run npm ci --production first (which emsdk does). Though that is not the point.

I don't think we should make it a goal for end users to be able to run the test suite

I don't want the end users to run the test suite. I want to run the test suite when I am bundling Emscripten, to hold Emscripten team to a guarantee that it has not accidentally added a dependency to a npm package that will not be shipped to end users.

Right now there is no way to verify that assumption won't have been violated.

I don't even think we should be shipping the test suite to end users by default.

Agreed. I am not shipping the test suite to end users.

sbc100 · 2025-05-29T17:13:50Z

everyone else who runs the test suite is doing so from a git checkout and running ./bootstrap first (which installs the dev dependencies).

Running bootstrap does not install dev dependencies if one has run npm ci --production first (which emsdk does). Though that is not the point.

I don't think thats quite how it works. npm ci --production will prune back the packages if you have already run npm ci. But running npm ci (without --production) which is what boostrap does will always install the dev depenednecies.

Maybe you are doing npm ci --production after the bootstrap process?

I don't think we should make it a goal for end users to be able to run the test suite

I don't want the end users to run the test suite. I want to run the test suite when I am bundling Emscripten, to hold Emscripten team to a guarantee that it has not accidentally added a dependency to a npm package that will not be shipped to end users.

Ok I can get on board with that, but that will mean that we need to add a new test configuration were we run as many tests as we can without the dev dependencies installed.

Right now we assume that dev dependencies are installed when running tests since we run all the tests out of a git checkout after installing dev dependnecies.

I guess really what we would need to do is run a make install to produce something more like what end users will see, then run the test suite against that.

Right now there is no way to verify that assumption won't have been violated.

I don't even think we should be shipping the test suite to end users by default.

Agreed. I am not shipping the test suite to end users.

sbc100 · 2025-05-29T17:16:05Z

This change seems to overlap with the existing npm_checked mechanism in test_sockets.py?

juj · 2025-05-29T17:21:31Z

npm ci --production will prune back the packages if you have already run npm ci.

Correct.

Maybe you are doing npm ci --production after the bootstrap process?

If one does

git clone https://github.com/emscripten-core/emsdk.git
cd emsdk
emsdk install sdk-main-64bit
emsdk activate sdk-main-64bit
cd emscripten\main
bootstrap.py // *
test\runner core0

then the starred line will not install dev dependencies, but is a no-op that says "up to date", because emsdk install sdk-main-64bit already ran npm ci --production.

But running npm ci (without --production) which is what boostrap does will always install the dev depenednecies.

That is not the case. bootstrap will say it is up to date, and not rerun npm ci after one has already run npm ci --production (like emsdk install does).

sbc100 · 2025-05-29T17:27:19Z

npm ci --production will prune back the packages if you have already run npm ci.

Correct.

Maybe you are doing npm ci --production after the bootstrap process?

If one does
git clone https://github.com/emscripten-core/emsdk.git
cd emsdk
emsdk install sdk-main-64bit
emsdk activate sdk-main-64bit
cd emscripten\main
bootstrap.py // *
test\runner core0
then the starred line will not install dev dependencies, but is a no-op that says "up to date", because emsdk install sdk-main-64bit already ran npm ci --production.

But running npm ci (without --production) which is what boostrap does will always install the dev depenednecies.

That is not the case. bootstrap will say it is up to date, and not rerun npm ci after one has already run npm ci --production (like emsdk install does).

Yes, that is to be expected. bootstrap uses a stamp file to mark the dependencies as installed. If you remove some of them (using npm ci --production after running bootstrap) it won't notice this.

BTW, does your emscripten\main contain a .git directory? Or is it built by some kind of install step (e.g make install AKA ./tools/install.py to produce that directory. I would assume what we really want to do here is run the tests against an installed version of emscripten without a .git directory?

sbc100

We should probably add a new circle CI config that tests this mode, but it can also be a followup.

LGTM % comments

test/common.py

tools/shared.py

juj · 2025-05-29T18:47:31Z

This change seems to overlap with the existing npm_checked mechanism in test_sockets.py?

Yeah, there is some amount of overlap here, though not sure I want to remove that npm_checked in sockets, since it globally checks for all tests in that file (which is convenient for that purpose).

sbc100 · 2025-05-29T18:52:52Z

This change seems to overlap with the existing npm_checked mechanism in test_sockets.py?

Yeah, there is some amount of overlap here, though not sure I want to remove that npm_checked in sockets, since it globally checks for all tests in that file (which is convenient for that purpose).

OK, maybe we can refactor as a followup.

juj · 2025-05-29T18:54:25Z

BTW, does your emscripten\main contain a .git directory? Or is it built by some kind of install step (e.g make install AKA ./tools/install.py to produce that directory. I would assume what we really want to do here is run the tests against an installed version of emscripten without a .git directory?

Currently what we do is 1:1 with emsdk:

git clone https://github.com/emscripten-core/emsdk.git
cd emsdk
emsdk install sdk-main-64bit --override-repository xxx --override-repository yyy --override-repository zzz
emsdk activate sdk-main-64bit
cd emscripten\main
rmdir /s /q <this and that and some other things under emscripten/ we don't want to ship to Unity users>
test\runner <all test suites we want to run>
perform a distribution of files we want to ship into /distributed_emsdk_root/ and zip it up

Though ideally we will want at some point want to refactor that to do

git clone https://github.com/emscripten-core/emsdk.git
cd emsdk
emsdk install sdk-main-64bit --override-repository xxx --override-repository yyy --override-repository zzz
emsdk activate sdk-main-64bit
perform a distribution of files we want to ship into /distributed_emsdk_root/
cd /distributed_emsdk_root/emscripten
test\runner <all test suites we want to run>
rmdir /s /q test
zip it up

so we would run tests in such a "make install"ed directory structure, and then remove trail of the test files.

juj · 2025-05-29T18:57:59Z

Ok, now ready for another review round.

sbc100 · 2025-05-29T19:03:32Z

Currently what we do is 1:1 with emsdk:

Sadly, one the problems is that out releases builder (and therefore almost all our users) don't install from source using emsdk.

We should probably work on unifying the two build processes. At the very least they should probably use the same install.py script to create packaged version of emscripten.

sbc100 · 2025-05-29T19:05:04Z

test/common.py

@@ -274,6 +274,23 @@ def decorated(self, *args, **kwargs):
  return decorated


+# Used to mark dependencies in various tests to npm developer dependency
+# packages, which might not be installed on Emscripten end users' systems.
+def requires_npm_package(package):


How about just calling this requires_dev_dependency?

Works for me. I presume I'd then change the env. var. from EMTEST_SKIP_NODE_DEV_PACKAGES to EMTEST_SKIP_DEV_PACKAGES or EMTEST_SKIP_DEV_DEPENDENCIES?

Add a new decorator @requires_npm_package('package-name') to allow di…

7a284e3

…sabling tests that require on a specific NPM development package. This allows running the test suite with production-only packages.

juj added 3 commits May 29, 2025 18:35

Fix npm expansion

62b1228

Ruff

7d65d11

XXX debug npm execution

2ddb94e

sbc100 reviewed May 29, 2025

View reviewed changes

Pass cwd() for check.

8101cbd

juj added 2 commits May 29, 2025 21:06

Simplify EMTEST_SKIP_NODE_DEV_PACKAGES mechanism

1c6bcbf

Add more requires_npm_package annotations.

5e2c52e

sbc100 reviewed May 29, 2025

View reviewed changes

test/common.py Outdated Show resolved Hide resolved

tools/shared.py Outdated Show resolved Hide resolved

Remove unnecessary code

2d0eeaf

Add comment

904731a

juj enabled auto-merge (squash) May 29, 2025 18:58

sbc100 approved these changes May 29, 2025

View reviewed changes

Rename requires_npm_package to requires_dev_dependency

3fa6424

juj merged commit d8d3280 into emscripten-core:main May 29, 2025
30 checks passed

Add a new decorator @requires_npm_package('package-name') #24430

Add a new decorator @requires_npm_package('package-name') #24430

Uh oh!

Conversation

juj commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juj commented May 29, 2025

Uh oh!

sbc100 commented May 29, 2025

Uh oh!

juj commented May 29, 2025

Uh oh!

sbc100 commented May 29, 2025

Uh oh!

sbc100 May 29, 2025

Choose a reason for hiding this comment

Uh oh!

sbc100 May 29, 2025

Choose a reason for hiding this comment

Uh oh!

juj May 29, 2025

Choose a reason for hiding this comment

Uh oh!

juj commented May 29, 2025

Uh oh!

sbc100 commented May 29, 2025

Uh oh!

sbc100 commented May 29, 2025

Uh oh!

juj commented May 29, 2025

Uh oh!

sbc100 commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

juj commented May 29, 2025

Uh oh!

sbc100 commented May 29, 2025

Uh oh!

juj commented May 29, 2025

Uh oh!

juj commented May 29, 2025

Uh oh!

sbc100 commented May 29, 2025

Uh oh!

sbc100 May 29, 2025

Choose a reason for hiding this comment

Uh oh!

juj May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

juj commented May 29, 2025 •

edited

Loading

sbc100 commented May 29, 2025 •

edited

Loading

sbc100 commented May 29, 2025 •

edited

Loading

juj May 29, 2025 •

edited

Loading