Make sockets suite more graceful to fail with user facing instructions #25290
Conversation
…s, and add env. var. EMTEST_LACKS_PYTHON_WEBSOCKIFY=1 to allow testing in shipping configuration.
juj
force-pushed
the
requires_python_websockify
branch
from
ba03ffb to
5f07bd4
Compare
test/test_sockets.py
Outdated
|
|
||
| npm_checked = False | ||
|
|
||
| EMTEST_LACKS_PYTHON_WEBSOCKIFY = int(os.getenv('EMTEST_LACKS_PYTHON_WEBSOCKIFY', '0')) |
There was a problem hiding this comment.
IIRC we decided its fine to the test suite to require pip install -r requirements-dev.txt?
In that case, I don't think we need this opt out? (same for psutil change you are working on, right?)
There was a problem hiding this comment.
(same for psutil change you are working on, right?)
psutil is currently installed for all users as a runtime dependency, and not just as a dev dependency.
websockify is not being shipped as a end user dependency.
I am testing the suites in end user configuration to ensure that nothing slips through the cracks that would require a dev dependency. So I want to skip all these tests (even though it does reduce coverage - that is ok for me)
There was a problem hiding this comment.
Ok, in that case perhaps we can re-use the existing requires_dev_dependency decorator and rename the corresponding env var from EMTEST_SKIP_NODE_DEV_PACKAGES to EMTEST_NO_DEV_PACKAGES?
There was a problem hiding this comment.
In the long run I wonder if we can perhaps try to find a way to run these extra tests without accidentally allowing emscripten itself to use these packages?
Perhaps we could find a way to force emscripten itself to use a hermetic python and node that doesn't have visibility of the dev packages (we could ever assert in emscripten itself that these packages are not visible). ?
There was a problem hiding this comment.
Ok, in that case perhaps we can re-use the existing
requires_dev_dependencydecorator and rename the corresponding env var fromEMTEST_SKIP_NODE_DEV_PACKAGEStoEMTEST_NO_DEV_PACKAGES?
Hmm, I'm not feeling so hot about that, I find this more documentative - distinguishing between node vs python packages is useful to find "the lay of the land", and also because the steps to install the dependencies are different (invoke npm install vs invoke pip install)
There was a problem hiding this comment.
Fair enough, but maybe at least use a single one for all python dev packages (like we do for node packages today).
There was a problem hiding this comment.
Hmm ok, maybe EMTEST_SKIP_PYTHON_DEV_PACKAGES then? Updated.
test/test_sockets.py
Outdated
| try: | ||
| import websockify # type: ignore | ||
| except ModuleNotFoundError: | ||
| raise Exception('Unable to import module websockify. Run "python3 -m pip install websockify" or set environment variable EMTEST_LACKS_PYTHON_WEBSOCKIFY=1 to skip this test.') from None |
There was a problem hiding this comment.
TIL about from None does that make the error easier to read?
Would exit_with_error() make it even more easy to ready in that case?
There was a problem hiding this comment.
Yes, the from None does make the error easier to read.
exit_with_error() would stop the whole harness from running.
2 participants
Make sockets suite more graceful to fail with user facing instructions, and add env. var. EMTEST_LACKS_PYTHON_WEBSOCKIFY=1 to allow testing in shipping configuration.