s2i-nodejs-container
s2i-nodejs-container copied to clipboard
sclorg
Unstable tests
Adding support for unstable tests.
Depends on https://github.com/sclorg/container-common-scripts/pull/268, thus not yet mergeable or testable in here.
Openshift tests would fail because of infra issues. The important tests for this PR are only the container ones. [test]
Update common/ to contain https://github.com/sclorg/container-common-scripts/pull/273.
[test-all]
@mhdawson Hi Michael, what do you mean about pino client? We have a proposal in this PR to mark pino tests as UNSTABLE FAILED, even if they failed. This means, that even if they fail, then the whole test suite will be marked as successful.
@phracek that sounds to me like the equivalent of removing the pino tests. They were added to support pino being part of the "Tested and Verified" list that RH has, so not doing something if the tests fail is a concern(which I think would be the case if they run still shows as passed). Maybe I'm misunderstanding?
(@mhdawson, I only want to note, that the test will be considered unstable only when a global switch IGNORE_UNSTABLE_TESTS is enabled. This is not a case of tests running in upstream sclorg repositories.)
@zmiklank thanks for the clarification. If I understand correctly we'll still be running the tests as part of the regular RHEL regression tests in which case I'm not as concerned.
@zmiklank Test is failed on CentOS Stream 9 here:
07:20:53 out: [PASSED] for 'hw' test_run_hw_application (00:00:01)
07:20:53 out: [PASSED] for 'hw' test_incremental_build (00:00:29)
07:20:53 out: [PASSED] for 'hw' test_build_express_webapp (00:00:22)
07:20:53 out: [PASSED] for 'clients' test_client_express (00:00:45)
07:20:53 out: [FAILED] for 'clients' test_client_pino (00:05:02)
07:20:53 out: [PASSED] for 'clients' test_client_prom (00:01:52)
07:20:53 out: [PASSED] for 'clients' test_client_opossum (00:01:26)
07:20:53 out: [PASSED] for 'clients' test_client_kube (00:00:38)
It looks like Unstable tests are not applied in the case of CentOS Stream 9.
Yes, they are not. We do not want to enable unstable tests for upstream testing as we want to be aware when the tests are failing.
We want to enable them only for downstream testing AFAIK. (When IGNORE_UNSTABLE_TESTS is enabled.)
Let me know, if otherwise, but this is my understanding of this matter.
If I understand correctly we'll still be running the tests as part of the regular RHEL regression tests in which case I'm not as concerned.
The test cases will be run, but the unstable test failures will be ignored. That means the pino test can be failing when we release the images.
The test cases will be run, but the unstable test failures will be ignored. That means the pino test can be failing when we release the images.
It means, that the test suite will fail in upstream repos, right? DId I get it correctly?
@zmiklank thanks for the clarification. If I understand correctly we'll still be running the tests as part of the regular RHEL regression tests in which case I'm not as concerned.
Hi @mhdawson , there are two tests. The first tests are upstream tests and the second one tests are dist-git or downstream tests. Both of them are the same and placed in the upstream repository in the directory test.
In the case of upstream tests if e.g. pino is failing, then the test failed and in upstream we will see, that the test for a specific host failed as well.
For downstream tests, e.g. pino is marked as unstable, because of pino's upstream did not fix it yet, and if the test fails, then the whole test suite for the specific host is marked as the test has passed.
Are you fine with this behavior? If pino test suite is not working for some pino's upstream an issue, I have filed issue already, but I can not find the reproducer, and the issue was closed https://github.com/pinojs/pino/issues/1252. In case we will have a reproducer, we can file an issue again.
I've tried to run pino tests outside of the container test suite and also got non-stable behaviour:
I've prepared the container like this:
$> podman run -ti --rm ubi8/nodejs-16 bash
bash$ npm show pino version
bash$ git clone https://github.com/pinojs/pino.git
bash$ cd pino/
bash$ git checkout v8.4.1
Then installing the pino failed the first time:
bash$ npm install
npm WARN deprecated [email protected]: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
npm ERR! code ERR_SOCKET_TIMEOUT
npm ERR! network Socket timeout
npm ERR! network This is a problem related to network connectivity.
npm ERR! network In most cases you are behind a proxy or have bad network settings.
npm ERR! network
npm ERR! network If you are behind a proxy, please make sure that the
npm ERR! network 'proxy' config is set properly. See: 'npm help config'
npm ERR! A complete log of this run can be found in:
npm ERR! /opt/app-root/src/.npm/_logs/2022-08-16T14_18_30_814Z-debug-0.log
...but succeeded when run again:
bash$ npm install
npm WARN deprecated [email protected]: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
added 1275 packages, and audited 1438 packages in 25s
146 packages are looking for funding
run `npm fund` for details
8 vulnerabilities (4 moderate, 4 high)
Some issues need review, and may require choosing
a different dependency.
Run `npm audit` for details.
Similarly, running the tests failed the first time:
bash$ npm test
> [email protected] test
> npm run lint && npm run transpile && tap --ts && jest test/jest && npm run test-types
> [email protected] lint
> eslint .
Oops! Something went wrong! :(
ESLint: 8.22.0
Error: Cannot find module 'estraverse'
Require stack:
- /opt/app-root/src/pino/node_modules/esrecurse/esrecurse.js
- /opt/app-root/src/pino/node_modules/eslint-scope/dist/eslint-scope.cjs
- /opt/app-root/src/pino/node_modules/eslint/lib/linter/linter.js
- /opt/app-root/src/pino/node_modules/eslint/lib/linter/index.js
- /opt/app-root/src/pino/node_modules/eslint/lib/cli-engine/cli-engine.js
- /opt/app-root/src/pino/node_modules/eslint/lib/eslint/eslint.js
- /opt/app-root/src/pino/node_modules/eslint/lib/eslint/index.js
- /opt/app-root/src/pino/node_modules/eslint/lib/cli.js
- /opt/app-root/src/pino/node_modules/eslint/bin/eslint.js
at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
at Function.Module._load (node:internal/modules/cjs/loader:778:27)
at Module.require (node:internal/modules/cjs/loader:1005:19)
at require (/opt/app-root/src/pino/node_modules/v8-compile-cache/v8-compile-cache.js:159:20)
at /opt/app-root/src/pino/node_modules/esrecurse/esrecurse.js:27:22
at Object.<anonymous> (/opt/app-root/src/pino/node_modules/esrecurse/esrecurse.js:116:2)
at Module._compile (/opt/app-root/src/pino/node_modules/v8-compile-cache/v8-compile-cache.js:192:30)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
at Module.load (node:internal/modules/cjs/loader:981:32)
at Function.Module._load (node:internal/modules/cjs/loader:822:12)
And then succeeded:
bash$ npm test
> [email protected] test
> npm run lint && npm run transpile && tap --ts && jest test/jest && npm run test-types
> [email protected] lint
> eslint .
> [email protected] transpile
> node ./test/fixtures/ts/transpile.cjs
SKIP test/http.test.js
~ http request support via serializer without request connection
SKIP test/http.test.js 1 skip of 16 229.563ms
~ http request support via serializer without request connection
Suites: 39 passed, 39 of 39 completed
Asserts: 1165 passed, 1 skip, of 1166
----------------------|---------|----------|---------|---------|-------------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------------------|---------|----------|---------|---------|-------------------------
All files | 97.56 | 95.77 | 97.58 | 97.68 |
pino | 100 | 100 | 100 | 100 |
browser.js | 100 | 100 | 100 | 100 |
file.js | 100 | 100 | 100 | 100 |
pino.js | 100 | 100 | 100 | 100 |
pino/lib | 96.59 | 93.94 | 96.7 | 96.78 |
caller.js | 86.66 | 50 | 100 | 86.66 | 14,23
levels.js | 100 | 100 | 100 | 100 |
meta.js | 100 | 100 | 100 | 100 |
multistream.js | 98.66 | 97.82 | 100 | 98.63 | 91
proto.js | 100 | 100 | 100 | 100 |
redaction.js | 100 | 100 | 100 | 100 |
symbols.js | 100 | 100 | 100 | 100 |
time.js | 100 | 100 | 100 | 100 |
tools.js | 95.97 | 96 | 88.88 | 95.8 | 221,253-255,289,302,351
transport-stream.js | 52.38 | 30 | 100 | 55.55 | 20-26,32-35
transport.js | 98.33 | 100 | 92.3 | 98.33 | 65
----------------------|---------|----------|---------|---------|-------------------------
PASS test/jest/basic.spec.js
✓ transport should work in jest (7 ms)
Test Suites: 1 passed, 1 total
Tests: 1 passed, 1 total
Snapshots: 0 total
Time: 0.921 s
Ran all test suites matching /test\/jest/i.
So, it looks like something is really unstable. What it is specifically, I don't know.
When you say the test failed the first time but then succeeded, was that after a successful install, ie no other steps between the first npm test and the second? The test failure looks like it failed to find a module that should have been installed.
Well, the install failed first, then succeeded when run in the same container the second time. So, maybe the install reported success, but something was wrong from the first unsuccessful atempt? Anyway, this is the full log or one container (ubi8/nodejs-16) run:
<skipped output of git clone pino... & git checkout v8.4.1>
bash-4.4$ npm install
npm WARN deprecated [email protected]: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
npm ERR! code ERR_SOCKET_TIMEOUT
npm ERR! network Socket timeout
npm ERR! network This is a problem related to network connectivity.
npm ERR! network In most cases you are behind a proxy or have bad network settings.
npm ERR! network
npm ERR! network If you are behind a proxy, please make sure that the
npm ERR! network 'proxy' config is set properly. See: 'npm help config'
npm ERR! A complete log of this run can be found in:
npm ERR! /opt/app-root/src/.npm/_logs/2022-08-16T13_26_13_978Z-debug-0.log
bash-4.4$ npm install
added 486 packages, removed 401 packages, changed 287 packages, and audited 1524 packages in 27s
140 packages are looking for funding
run `npm fund` for details
8 vulnerabilities (4 moderate, 4 high)
Some issues need review, and may require choosing
a different dependency.
Run `npm audit` for details.
bash-4.4$ npm test
> [email protected] test
> npm run lint && npm run transpile && tap --ts && jest test/jest && npm run test-types
> [email protected] lint
> eslint .
Oops! Something went wrong! :(
ESLint: 8.22.0
Error: Cannot find module 'estraverse'
Require stack:
- /opt/app-root/src/pino/node_modules/esrecurse/esrecurse.js
- /opt/app-root/src/pino/node_modules/eslint-scope/dist/eslint-scope.cjs
- /opt/app-root/src/pino/node_modules/eslint/lib/linter/linter.js
- /opt/app-root/src/pino/node_modules/eslint/lib/linter/index.js
- /opt/app-root/src/pino/node_modules/eslint/lib/cli-engine/cli-engine.js
- /opt/app-root/src/pino/node_modules/eslint/lib/eslint/eslint.js
- /opt/app-root/src/pino/node_modules/eslint/lib/eslint/index.js
- /opt/app-root/src/pino/node_modules/eslint/lib/cli.js
- /opt/app-root/src/pino/node_modules/eslint/bin/eslint.js
at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
at Function.Module._load (node:internal/modules/cjs/loader:778:27)
at Module.require (node:internal/modules/cjs/loader:1005:19)
at require (/opt/app-root/src/pino/node_modules/v8-compile-cache/v8-compile-cache.js:159:20)
at /opt/app-root/src/pino/node_modules/esrecurse/esrecurse.js:27:22
at Object.<anonymous> (/opt/app-root/src/pino/node_modules/esrecurse/esrecurse.js:116:2)
at Module._compile (/opt/app-root/src/pino/node_modules/v8-compile-cache/v8-compile-cache.js:192:30)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
at Module.load (node:internal/modules/cjs/loader:981:32)
at Function.Module._load (node:internal/modules/cjs/loader:822:12)
Are you fine with this behavior? If pino test suite is not working for some pino's upstream an issue, I have filed issue already, but I can not find the reproducer, and the issue was closed https://github.com/pinojs/pino/issues/1252. In case we will have a reproducer, we can file an issue again.
I'm not sure I understand the situation yet. Would it be ok if I set up a short call to discuss with you? That might help close the gap in my understanding faster.
@hhorak thanks for the full log.
My feeling is that the test failure is still and artifact of the initial timeout in the first install. I agree the second npm install should have fixed that up, but its probably more useful to look at the why the timeout occurs if that causes most of the flakiness.
In terms of the second install, do our scripts try to do a second install if the first fails or was that just part of your testing. If the scripts do a retry, then it might be possible to add some cleanup so that the second install starts from scratch.
@mhdawson @hhorak I would say, we have a reproducer. I will try it on my system too and file the issue to pino again with full log and traces. I guess this should be handled on pino side.
@phracek if you can at mention me in the pino issue once you open it, I'll work to have our team work to move it forward.
I've divided this PR into two separate ones, leaving here only addition of pino test to the set of unstable tests. This PR is now blocked by https://github.com/sclorg/s2i-nodejs-container/pull/356.
@mhdawson Not yet. I was trying to do the same steps as @hhorak did, but all were successful.
$ podman pull registry.redhat.io/ubi8/nodejs-16
$ podman run -it --rm registry.redhat.io/ubi8/nodejs-16 bash
$ npm show pino version
8.6.1
$ git clone https://github.com/pinojs/pino.git
$ cd pino
$ git checkout v8.4.1
Note: switching to 'v8.4.1'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.
If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:
git switch -c <new-branch-name>
Or undo this operation with:
git switch -
Turn off this advice by setting config variable advice.detachedHead to false
HEAD is now at 7525e3e Bumped v8.4.1
$ npm install
npm WARN deprecated [email protected]: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
added 1276 packages, and audited 1439 packages in 3m
152 packages are looking for funding
run `npm fund` for details
8 vulnerabilities (7 moderate, 1 high)
To address issues that do not require attention, run:
npm audit fix
To address all issues (including breaking changes), run:
npm audit fix --force
Run `npm audit` for details.
npm notice
npm notice New minor version of npm available! 8.11.0 -> 8.19.2
npm notice Changelog: https://github.com/npm/cli/releases/tag/v8.19.2
npm notice Run npm install -g [email protected] to update!
npm notice
$ npm test
> [email protected] test
> npm run lint && npm run transpile && tap --ts && jest test/jest && npm run test-types
> [email protected] lint
> eslint .
> [email protected] transpile
> node ./test/fixtures/ts/transpile.cjs
SKIP test/http.test.js
~ http request support via serializer without request connection
SKIP test/http.test.js 1 skip of 16 183.132ms
~ http request support via serializer without request connection
Suites: 39 passed, 39 of 39 completed
Asserts: 1165 passed, 1 skip, of 1166
----------------------|---------|----------|---------|---------|-------------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------------------|---------|----------|---------|---------|-------------------------
All files | 97.56 | 95.77 | 97.58 | 97.68 |
pino | 100 | 100 | 100 | 100 |
browser.js | 100 | 100 | 100 | 100 |
file.js | 100 | 100 | 100 | 100 |
pino.js | 100 | 100 | 100 | 100 |
pino/lib | 96.59 | 93.94 | 96.7 | 96.78 |
caller.js | 86.66 | 50 | 100 | 86.66 | 14,23
levels.js | 100 | 100 | 100 | 100 |
meta.js | 100 | 100 | 100 | 100 |
multistream.js | 98.66 | 97.82 | 100 | 98.63 | 91
proto.js | 100 | 100 | 100 | 100 |
redaction.js | 100 | 100 | 100 | 100 |
symbols.js | 100 | 100 | 100 | 100 |
time.js | 100 | 100 | 100 | 100 |
tools.js | 95.97 | 96 | 88.88 | 95.8 | 221,253-255,289,302,351
transport-stream.js | 52.38 | 30 | 100 | 55.55 | 20-26,32-35
transport.js | 98.33 | 100 | 92.3 | 98.33 | 65
----------------------|---------|----------|---------|---------|-------------------------
PASS test/jest/basic.spec.js
✓ transport should work in jest (11 ms)
Test Suites: 1 passed, 1 total
Tests: 1 passed, 1 total
Snapshots: 0 total
Time: 1.574 s
Ran all test suites matching /test\/jest/i.
> [email protected] test-types
> tsc && tsd && ts-node test/types/pino.ts
It looks like pino started to work again with latest version:
$ podman run -it --rm registry.access.redhat.com/ubi8/nodejs-16 bash
bash-4.4$ npm show pino version
8.7.0
bash-4.4$ git clone https://github.com/pinojs/pino.git
Cloning into 'pino'...
remote: Enumerating objects: 7822, done.
remote: Counting objects: 100% (157/157), done.
remote: Compressing objects: 100% (84/84), done.
remote: Total 7822 (delta 91), reused 126 (delta 73), pack-reused 7665
Receiving objects: 100% (7822/7822), 2.72 MiB | 273.00 KiB/s, done.
Resolving deltas: 100% (5405/5405), done.
bash-4.4$ cd pino
bash-4.4$ git checkout v8.7.0
Note: switching to 'v8.7.0'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.
If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:
git switch -c <new-branch-name>
Or undo this operation with:
git switch -
Turn off this advice by setting config variable advice.detachedHead to false
HEAD is now at 085401b Bumped v8.7.0
bash-4.4$ npm install
npm WARN deprecated [email protected]: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
added 1275 packages, and audited 1438 packages in 1m
154 packages are looking for funding
run `npm fund` for details
8 vulnerabilities (7 moderate, 1 high)
To address issues that do not require attention, run:
npm audit fix
To address all issues (including breaking changes), run:
npm audit fix --force
Run `npm audit` for details.
npm notice
npm notice New minor version of npm available! 8.11.0 -> 8.19.2
npm notice Changelog: https://github.com/npm/cli/releases/tag/v8.19.2
npm notice Run npm install -g [email protected] to update!
npm notice
bash-4.4$ npm test
> [email protected] test
> npm run lint && npm run transpile && tap --ts && jest test/jest && npm run test-types
> [email protected] lint
> eslint .
> [email protected] transpile
> node ./test/fixtures/ts/transpile.cjs
SKIP test/http.test.js
~ http request support via serializer without request connection
SKIP test/http.test.js 1 skip of 16 157.886ms
~ http request support via serializer without request connection
Suites: 39 passed, 39 of 39 completed
Asserts: 1182 passed, 1 skip, of 1183
----------------------|---------|----------|---------|---------|---------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------------------|---------|----------|---------|---------|---------------------
All files | 97.57 | 95.8 | 97.58 | 97.56 |
pino | 100 | 100 | 100 | 100 |
browser.js | 100 | 100 | 100 | 100 |
file.js | 100 | 100 | 100 | 100 |
pino.js | 100 | 100 | 100 | 100 |
pino/lib | 96.6 | 94.01 | 96.7 | 96.62 |
caller.js | 86.66 | 50 | 100 | 86.66 | 14,23
levels.js | 100 | 100 | 100 | 100 |
meta.js | 100 | 100 | 100 | 100 |
multistream.js | 98.66 | 97.82 | 100 | 98.63 | 91
proto.js | 100 | 100 | 100 | 100 |
redaction.js | 100 | 100 | 100 | 100 |
symbols.js | 100 | 100 | 100 | 100 |
time.js | 100 | 100 | 100 | 100 |
tools.js | 96.55 | 96.71 | 88.88 | 96.38 | 221,253-255,289,349
transport-stream.js | 52.17 | 31.81 | 100 | 50 | 20-26,32-38
transport.js | 98.33 | 100 | 92.3 | 98.33 | 65
----------------------|---------|----------|---------|---------|---------------------
PASS test/jest/basic.spec.js
✓ transport should work in jest (5 ms)
Test Suites: 1 passed, 1 total
Tests: 1 passed, 1 total
Snapshots: 0 total
Time: 0.709 s
Ran all test suites matching /test\/jest/i.
> [email protected] test-types
> tsc && tsd && ts-node test/types/pino.ts
[12:03:00.456] INFO (855): test2
[12:03:00.454] INFO (855): test2
some: "bindings"
[12:03:00.457] INFO (855): test2
bash-4.4$ exit
https://github.com/sclorg/container-common-scripts/pull/268 is merged, so this PR is no longer blocked on it.