Remove Node.js self.run_js() test in browser suite.

[juj]

Remove Node.js self.run_js() test in browser suite. The browser suite should test browser behavior only.

I am running the browser suite with

EMCC_CFLAGS= -sMIN_CHROME_VERSION=-1 -sMIN_SAFARI_VERSION=-1 -sMIN_NODE_VERSION=-1

to verify that targeting Firefox only should correctly result in a build that works in Firefox.

The test test_hello_world_worker is the only one that failed. Which happens because that test also verifies behavior in node.js.

Reading the test logic in that test, it seems to verify the file packager and emscripten_run_script() logic. That logic should be well covered in the core test suites and the other test suite already, so this seems redundant.

[sbc100]

Is there some particular reason you want to remove this? Did it causes issues somehow?

[juj]

The rationale is that it prevents testing the said configuration. And given it looks redundant, I thought rather simpler to remove it, than to have a manual test skip. Is there anything relevant that this code tests that isn't tested elsewhere?

[sbc100]

The rationale is that it prevents testing the said configuration

Can you explain this?

[juj]

I am setting the environment variable

EMCC_CFLAGS= -sMIN_CHROME_VERSION=-1 -sMIN_SAFARI_VERSION=-1 -sMIN_NODE_VERSION=-1 -sMIN_FIREFOX_VERSION=<firefox_version_under_test>

before running test/runner browser

so that all the tests would focus on verifying that MIN_FIREFOX_VERSION is accurate to the target Firefox version.

But because the one test browser.test_hello_world_worker also tries verifying behavior in Node.js, then adding those CFLAGS results in generating a build that doesn't run in Node, but aborts because that was not in the target environment list.

That one line is the only outlier in the whole browser test harness that attempts to test Node.js behavior, rest of the suite passes.

[sbc100]

Oh I think I see. We set EMTEST_SKIP_NODE_CANARY: "1" when we run the 4gb browser tests to avoid this. See test-browser-chrome-wasm64-4gb in .circleci/config.yml.

Maybe you could do that same in your CI? Or if not then maybe also remove the EMTEST_SKIP_NODE_CANARY: "1" from config.yml as part of this PR?

[juj]

I get this issue in all the browser suites, not just the 4GB suite.

[juj]

I do already pass EMTEST_SKIP_NODE_CANARY=1 in the affected browser runs. (Maybe there will be more of these if that is not enabled?)

[sbc100]

-sMIN_NODE_VERSION=-1

Do you need to inject this flag? It seems like that would mean you are testing a slightly different configuration than the one the test suite it setup for, no? Why not just leave MIN_NODE_VERSION to its default value?

[juj]

The idea is to improve test coverage by setting flags that only target Firefox. It should be a valid combination of configuration for end users to pass, so "why are you testing that. Don't do it" doesn't seem a good resolution.

[sbc100]

Is specifying -sMIN_NODE_VERSION=-1 the same as specifying a -sENVIRONMENT that doesn't contain node at all? IIUC it is? That seems like you would be testing a configuration that is more then just "support a specific FF version", its also testing the that whole browser test suite can be built with -sENVIRONMENT=web only, which is maybe useful but maybe separate to the support for older FF versions.

[juj]

Is specifying -sMIN_NODE_VERSION=-1 the same as specifying a -sENVIRONMENT that doesn't contain node at all?

No, because there is also -sENVIRONMENT=shell. (or I guess maybe)

The reason I am only enabling Firefox in the test run is to make sure that supporting some other environment does not accidentally mask and enable a feature, that would hide a bug with the minimum version levels for Firefox (like e.g. in #25465)

[sbc100]

Removing this seems reasonable on brief inspection yes

I didn't write this test though so I'm not sure exactly what its trying to verify. It looks like it goes back to #5011, or even before.

@kripken WDTY?

[sbc100]

Is specifying -sMIN_NODE_VERSION=-1 the same as specifying a -sENVIRONMENT that doesn't contain node at all?

No, because there is also -sENVIRONMENT=shell. (or I guess maybe)

I'm not sure I understand. -sENVIRONMENT=shell does not support node, right? I think shell only refers to things like v8/spidermonkey/jsc.

[juj]

-sENVIRONMENT=shell does not support node, right?

Yeah that's what I was confused about.. I was not sure if it includes node or not.. but probably not.

[juj]

@kripken would this be ok to land? The deleted block of code is an oddity in the browser suite: it verifies behavior in the Node.js shell rather than the browser.

To my eyes, the deleted code amounts to verifying that

[EMCC, test_file('hello_world_worker.c'), '-o', 'worker.js', '--preload-file', 'file.dat'] + self.get_cflags()
self.run_process(cmd)
self.assertExists('worker.js')
self.assertContained('you should not see this text when in a worker!', self.run_js('worker.js'))

works. That doesn't seem like anything special/relevant? We do have a lot of tests for --preload-file in the other suite, for example other.test_include_file?

By deleting that code, I can run the browser suite in a special mode with

EMCC_CFLAGS= -sMIN_FIREFOX_VERSION=143 -sMIN_CHROME_VERSION=-1 -sMIN_SAFARI_VERSION=-1 -sMIN_NODE_VERSION=-1

[sbc100]

Can you also remove the corresponding EMTEST_SKIP_NODE_CANARY entries from the test-browser-* tasks in .circleci/config.yml?

[juj]

Hmm how do you mean? I don't see a reference to browser.test_hello_world_worker in config.yml?

[juj]

I'm not sure if this PR will necessarily allow removing EMTEST_SKIP_NODE_CANARY, that seems unrelated?

[sbc100]

I'm not sure if this PR will necessarily allow removing EMTEST_SKIP_NODE_CANARY, that seems unrelated?

We currently set EMTEST_SKIP_NODE_CANARY in test-browser-chrome-wasm64-4gb specifically because if we don't this test would fail since it has require_node_canary in it.

[juj]

Oh I see, that was the only test remaining in browser suite that had the canary part.