fix substring search in tests #378

pkubatrh · 2023-02-03T10:44:47Z

No description provided.

pkubatrh · 2023-02-03T10:48:20Z

[test-all]

pkubatrh · 2023-02-07T13:11:31Z

[test-all]

phracek · 2023-02-07T13:54:34Z

@zmiklank This could be valid also for the other containers.

phracek · 2023-02-07T14:56:29Z

The subscription should be solved now.

[test-all]

pkubatrh · 2023-02-08T08:06:06Z

Seems like pino and prom are failing all across the board.
@mhdawson any idea what that might be caused by?

pkubatrh · 2023-02-08T11:31:15Z

[test-all]

Edit: Seems like only prom is failing now.

zmiklank · 2023-02-09T16:27:58Z

Sorry, I did not manage to look at this properly today. Will do it on monday.

mhdawson · 2023-02-09T20:46:55Z

@lholmquist mentioned that he had run the prom-client tests locally and they all passed.

mhdawson · 2023-02-09T20:49:12Z

Can you point us to a run with prom failing ?

pkubatrh · 2023-02-10T07:00:23Z

@mhdawson All the recent test runs failed. Eg. Fedora with Node 18 - https://artifacts.dev.testing-farm.io/9ba56c06-618d-4b76-9b47-44ad25fca646/

mhdawson · 2023-02-10T19:44:57Z

Thanks it was just not obvious to me where to find the runs from this issue.

I'll try to look on Monday at the failures in the runs to see what's going on.

mhdawson · 2023-02-13T20:07:11Z

First step - Tests all passed when run locally in Fedora 37 and Node.js 16.18.1

mhdawson · 2023-02-13T21:42:38Z

This is the failure:

Run `npm audit` for details.
npm notice 
npm notice New major version of npm available! 8.8.0 -> 9.4.2
npm notice Changelog: <https://github.com/npm/cli/releases/tag/v9.4.2>
npm notice Run `npm install -g npm@9.4.2` to update!
npm notice 
--> 53d9ed5b2a8
STEP 9/9: CMD /usr/libexec/s2i/run
COMMIT quay.io/fedora/nodejs-18:18-prom-client
--> e402f2a968b
Successfully tagged quay.io/fedora/nodejs-18:18-prom-client
e402f2a968b2bef2a14d194ad557636025d5f6901fa0756a2b59f5d846b8f74c

> prom-client@14.1.1 test /opt/app-root/src
> npm run lint && npm run check-prettier && npm run compile-typescript && npm run test-unit


> prom-client@14.1.1 lint /opt/app-root/src
> eslint .


> prom-client@14.1.1 check-prettier /opt/app-root/src
> npm run run-prettier -- --check


> prom-client@14.1.1 run-prettier /opt/app-root/src
> prettier . .eslintrc "--check"

Checking formatting...
[warn] .config/configstore/update-notifier-npm.json
[warn] Code style issues found in the above file. Forgot to run Prettier?
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! prom-client@14.1.1 run-prettier: `prettier . .eslintrc "--check"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the prom-client@14.1.1 run-prettier script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /opt/app-root/src/.npm/_logs/2023-02-08T11_56_37_211Z-debug.log
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! prom-client@14.1.1 check-prettier: `npm run run-prettier -- --check`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the prom-client@14.1.1 check-prettier script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /opt/app-root/src/.npm/_logs/2023-02-08T11_56_37_232Z-debug.log
npm ERR! Test failed.  See above for more details.
S2I image 'quay.io/fedora/nodejs-18:18' test FAILED (exit code: 1)
Test for image 'quay.io/fedora/nodejs-18:18' FAILED (exit code: 1)

mhdawson · 2023-02-13T21:43:19Z

Looks like it is not failing running any of the tests but instead on style check


Checking formatting...
[warn] .config/configstore/update-notifier-npm.json
[warn] Code style issues found in the above file. Forgot to run Prettier?
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! prom-client@14.1.1 run-prettier: `prettier . .eslintrc "--check"`
npm ERR! Exit status 1

mhdawson · 2023-02-13T21:44:39Z

I don't see the file it is complaining about in the github repo (https://github.com/siimon/prom-client
)

.config/configstore/update-notifier-npm.json

mhdawson · 2023-02-13T21:53:39Z

@pkubatrh @phracek are the instructions for running the tests locally up to date?

I've tried both with podman and with docker running

make test TARGET=rhel8 VERSION=16

with docker I can't get it to download the containers, it just fails telling me it can't download ubi8/ubi-minimal.

with podman I can get further but it still fails long before it starts to run the tests for the JavaScript packages.

mhdawson · 2023-02-13T22:27:26Z

I can recreate the problem with this Dockerfile:

FROM  registry.access.redhat.com/ubi8/nodejs-16@sha256:1127129cb991f0be842e241265f0312642f36ec87f4a5a124d612468a7d966ea
LABEL "io.openshift.s2i.build.image"="quay.io/fedora/nodejs-18:18"
USER root
COPY ./prom-client /tmp/src
RUN chown -R 1001:0 /tmp/src
ENV NODE_ENV=development
USER 1001
RUN /usr/libexec/s2i/assemble

If I then do docker run -it mytest /bin/bash and then npm test I see a similarl failure:

user1@fedora check]$ docker run -it mytest /bin/bash
bash-4.4$ npm test

> prom-client@14.1.1 test /opt/app-root/src
> npm run lint && npm run check-prettier && npm run compile-typescript && npm run test-unit


> prom-client@14.1.1 lint /opt/app-root/src
> eslint .


> prom-client@14.1.1 check-prettier /opt/app-root/src
> npm run run-prettier -- --check


> prom-client@14.1.1 run-prettier /opt/app-root/src
> prettier . .eslintrc "--check"

Checking formatting...
[warn] .config/configstore/update-notifier-npm.json
[warn] .npm/anonymous-cli-metrics.json
[warn] Code style issues found in 2 files. Forgot to run Prettier?
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! prom-client@14.1.1 run-prettier: `prettier . .eslintrc "--check"`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the prom-client@14.1.1 run-prettier script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /opt/app-root/src/.npm/_logs/2023-02-13T22_27_05_950Z-debug.log
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! prom-client@14.1.1 check-prettier: `npm run run-prettier -- --check`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the prom-client@14.1.1 check-prettier script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /opt/app-root/src/.npm/_logs/2023-02-13T22_27_05_962Z-debug.log
npm ERR! Test failed.  See above for more details.
bash-4.4$

mhdawson · 2023-02-13T22:48:19Z

If I update my Docker file to be:

FROM  registry.access.redhat.com/ubi8/nodejs-16@sha256:1127129cb991f0be842e241265f0312642f36ec87f4a5a124d612468a7d966ea
LABEL "io.openshift.s2i.build.image"="quay.io/fedora/nodejs-18:18"
USER root
COPY ./prom-client /tmp/src
COPY ./prom-client /tmp/src/package-test
RUN mkdir -p /tmp/src/package-test
RUN chown -R 1001:0 /tmp/src
ENV NODE_ENV=development
USER 1001
RUN /usr/libexec/s2i/assemble/opt/app-root/src/package-test

That gives me two copies of the prom-client clone from GitHub.

One which just has the contents from GitHub in /opt/app-root/src/package-test
One which has the contensts from GithHub + whatever assemble generates.

If I run npm test in /opt/app-root/src/package-test then the tests run/pass ok

If I run npm test in the poluted /opt/app-root/src then I see the complaint from the prettier check.

This all makes sense to me. The tests for prom/opt/app-root/srcu-client don't expect a bunch of extra files generated by s2i when it checks the files in the directory.

I think the tests which are run should not run from the /opt/app-root/src as that is asking for these kinds of problems because of the additional files. Instead they should be run from a directory that only includes what was checked out from GitHub and the result of an npm install. The second copy I made seems to work as it finds the node_modules in the parent directly where s2i did the install, but it does not have any extraneous files.

The net is that I think the test framework needs to be fixed in order to resolve this.

mhdawson · 2023-02-13T22:50:13Z

I'd still like to be able to run the tests locally so if you can help me do that I'll be able to investigate issues more easily in the future. I was able to do that in the past but not sure if there are new dependencies or what that prevent the make test TARGET=rhel8 VERSION=16 for working for me.

pkubatrh · 2023-02-14T10:01:45Z

@mhdawson thanks for looking into this!

I do wonder how to tackle this issue with polluted directory. The way I see it we still want to test if the package works with our assemble scripts right?
Instead of moving the client repo elsewhere, is there some way to perhaps waive/skip the style check test cases? Does not seem like those would be something we want to gate a PR on.
Or, since it seems like its a npm file causing trouble, is there a way for the npm command to clean up after itself, so we do not end up in the polluted directory situation in the first place?

For the local build of the image - I expect both docker and podman should work, as we still have (afaik) docker on RHEL7-based machines during CI. The command you are using for it looks fine as well.
Note that I am usually running the builds and test as a root user, not sure if they can work the same way in a rootless environment. It would help to diagnose your build/test issues if you sent me the logs with the issue you encountered next time you try it out.

pkubatrh · 2023-02-14T10:06:01Z

As a npm noobie, looking at prom's package.json (https://github.com/siimon/prom-client/blob/master/package.json#L16) I see we might be able to get around the issue by patching out the npm run check-prettier part of the test script. Is my expectation correct?

Edit: In the meantime I created such a patch and seems like it does the trick for me locally.

pkubatrh · 2023-02-14T10:59:39Z

[test-all]

zmiklank · 2023-02-14T11:16:24Z

For the local build of the image - I expect both docker and podman should work, as we still have (afaik) docker on RHEL7-based machines during CI. The command you are using for it looks fine as well.

Yes, both works, however, I think the command should be make test TARGET=rhel8 VERSIONS=16. (VERSION vs VERSIONS)

pkubatrh · 2023-02-14T11:57:43Z

[test-all]

lholmquist · 2023-02-14T15:07:29Z

Looks like that fixed the prom-client tests, but now pino has 1 failing tests 🤦

mhdawson · 2023-02-14T20:01:28Z

Yes, both works, however, I think the command should be make test TARGET=rhel8 VERSIONS=16. (VERSION vs VERSIONS)

I had tried with both VERSION and VERSIONS

mhdawson · 2023-02-14T20:03:00Z

we might be able to get around the issue by patching out the npm run check-prettier part of the test script. Is my expectation correct?

I think that is good as a short term solution, but patching is going to be more work long term and we may run into the same issue with other packages. Figuring out a way to build/test when running npm test for packages that does not add all of the s2i files to where the package contents are would be a better long terms solution.

mhdawson · 2023-02-14T20:04:45Z

It would help to diagnose your build/test issues if you sent me the logs with the issue you encountered next time you try it out.

@pkubatrh. Thanks for the offer will send you my file through email and if that is not enough I'll schedule a time to get together and maybe you can get me going.

mhdawson · 2023-02-14T20:44:26Z

The pino tests pass running both locally and in a container for me. Not sure what about the test environment could be so different that the failure occurs on multiple containers.

pkubatrh · 2023-02-15T07:37:14Z

Last time I saw the pino failure, it was gone in the next run. Passing locally for me as well in a random combination out of the ones that failed in CI.
Lets try rerunning
[test-all]

phracek · 2023-02-15T13:44:34Z

The new cluster is up and ready. Let's try only OpenShift tests.

[test-openshift]

phracek

LGTM as well.

pkubatrh · 2023-02-16T09:49:49Z

On RHEL9 it seems like pino is failing in some different way:

Oops! Something went wrong! :(

ESLint: 8.34.0

TypeError: Cannot read properties of undefined (reading 'type')
Occurred while linting /opt/app-root/src/benchmarks/basic.bench.js:1
    at /opt/app-root/src/node_modules/esquery/dist/esquery.min.js:1:29469
    at /opt/app-root/src/node_modules/esquery/dist/esquery.min.js:1:30512
    at Function.b.matches (/opt/app-root/src/node_modules/esquery/dist/esquery.min.js:1:34848)
    at NodeEventGenerator.applySelector (/opt/app-root/src/node_modules/eslint/lib/linter/node-event-generator.js:296:21)
    at NodeEventGenerator.applySelectors (/opt/app-root/src/node_modules/eslint/lib/linter/node-event-generator.js:324:22)
    at NodeEventGenerator.enterNode (/opt/app-root/src/node_modules/eslint/lib/linter/node-event-generator.js:340:14)
    at CodePathAnalyzer.enterNode (/opt/app-root/src/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:795:23)
    at /opt/app-root/src/node_modules/eslint/lib/linter/linter.js:1153:32
    at Array.forEach (<anonymous>)
    at runRules (/opt/app-root/src/node_modules/eslint/lib/linter/linter.js:1148:15)
S2I image 'ubi9/nodejs-16:1' test FAILED (exit code: 2)
Test for image 'ubi9/nodejs-16:1' FAILED (exit code: 2)

Any ideas? I managed to recreate the issue locally in a RHEL9 VM this time.

pkubatrh · 2023-02-21T13:18:01Z

Different failure when rerunning the tests today

FAIL  test/syncfalse.test.js
 ✖ should be equal

  test/syncfalse.test.js
  50 |   await once(child, 'close')
  51 |   equal(actual, expected)
> 52 |   equal(actual2.trim(), expected2)
     | --^
  53 |
  54 |   teardown(() => {
  55 |     os.hostname = hostname

  test: test/syncfalse.test.js asynchronous logging
  found: ""
  wanted: '{"level":30,"time":1459875739796,"pid":123456,"hostname":"abcdefghijk
lmnopqr","msg":"h"}'
  compare: ===
  stack: |
    Test.<anonymous> (test/syncfalse.test.js:52:3)

mhdawson · 2023-02-21T22:22:03Z

@pkubatrh I assume it never passed on RHEL9?

I've tried

ubi8/nodejs-16 - tests all pass
ubi9/nodejs-16 (latest) - the reported tests fails, possibly also some hangs
ubi9/nodejs-16(ealiest container) - the reported tests fails
ubi9/nodejs-16 (latest) + add community Node.js to path - passes all tests.

So I can't find a ubi9 where that test ever passed. It also seems to be something about the RH Node.js as the test passes with community node.js in the same container were we see failures with the RH Node.js

pkubatrh · 2023-02-23T13:47:44Z

Opened a bug report for it: https://bugzilla.redhat.com/show_bug.cgi?id=2172910

Since it seems this test case never managed to pass for rhel9 nodejs, let's just merge it and make sure it gets fixed in future.

pkubatrh mentioned this pull request Feb 3, 2023

chore: Add the faas-js-runtime to the testing suite. #377

Merged

pkubatrh force-pushed the fix_client_tests branch from ac2a7cd to 764100b Compare February 8, 2023 11:30

zmiklank force-pushed the fix_client_tests branch from f7c65a5 to 266bec3 Compare February 13, 2023 09:14

zmiklank approved these changes Feb 13, 2023

View reviewed changes

pkubatrh added 3 commits February 14, 2023 12:57

fix substring search in tests

cef7a95

check for failure when running prepare

e7be074

remove leftover variable from change of testrunner

c6b3b0c

test: skip style-check for prom-client

368c69b

pkubatrh force-pushed the fix_client_tests branch from 8ff4ae3 to 368c69b Compare February 14, 2023 11:57

phracek approved these changes Feb 15, 2023

View reviewed changes

pkubatrh merged commit cd5a996 into sclorg:master Feb 23, 2023
20 of 25 checks passed

pkubatrh deleted the fix_client_tests branch February 23, 2023 13:47

fix substring search in tests #378

fix substring search in tests #378

pkubatrh commented Feb 3, 2023

pkubatrh commented Feb 3, 2023

pkubatrh commented Feb 7, 2023

phracek commented Feb 7, 2023

phracek commented Feb 7, 2023

pkubatrh commented Feb 8, 2023

pkubatrh commented Feb 8, 2023 •

edited

zmiklank commented Feb 9, 2023

mhdawson commented Feb 9, 2023

mhdawson commented Feb 9, 2023

pkubatrh commented Feb 10, 2023

mhdawson commented Feb 10, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

pkubatrh commented Feb 14, 2023 •

edited

pkubatrh commented Feb 14, 2023 •

edited

pkubatrh commented Feb 14, 2023

zmiklank commented Feb 14, 2023

pkubatrh commented Feb 14, 2023

lholmquist commented Feb 14, 2023

mhdawson commented Feb 14, 2023

mhdawson commented Feb 14, 2023

mhdawson commented Feb 14, 2023

mhdawson commented Feb 14, 2023

pkubatrh commented Feb 15, 2023

phracek commented Feb 15, 2023

phracek left a comment

pkubatrh commented Feb 16, 2023

pkubatrh commented Feb 21, 2023

mhdawson commented Feb 21, 2023

pkubatrh commented Feb 23, 2023

fix substring search in tests #378

fix substring search in tests #378

Conversation

pkubatrh commented Feb 3, 2023

pkubatrh commented Feb 3, 2023

pkubatrh commented Feb 7, 2023

phracek commented Feb 7, 2023

phracek commented Feb 7, 2023

pkubatrh commented Feb 8, 2023

pkubatrh commented Feb 8, 2023 • edited

zmiklank commented Feb 9, 2023

mhdawson commented Feb 9, 2023

mhdawson commented Feb 9, 2023

pkubatrh commented Feb 10, 2023

mhdawson commented Feb 10, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

mhdawson commented Feb 13, 2023

pkubatrh commented Feb 14, 2023 • edited

pkubatrh commented Feb 14, 2023 • edited

pkubatrh commented Feb 14, 2023

zmiklank commented Feb 14, 2023

pkubatrh commented Feb 14, 2023

lholmquist commented Feb 14, 2023

mhdawson commented Feb 14, 2023

mhdawson commented Feb 14, 2023

mhdawson commented Feb 14, 2023

mhdawson commented Feb 14, 2023

pkubatrh commented Feb 15, 2023

phracek commented Feb 15, 2023

phracek left a comment

Choose a reason for hiding this comment

pkubatrh commented Feb 16, 2023

pkubatrh commented Feb 21, 2023

mhdawson commented Feb 21, 2023

pkubatrh commented Feb 23, 2023

pkubatrh commented Feb 8, 2023 •

edited

pkubatrh commented Feb 14, 2023 •

edited

pkubatrh commented Feb 14, 2023 •

edited