Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: check version strings have expected pattern #23679

Closed

Conversation

sam-github
Copy link
Contributor

Many were checked, but a few were not.

Should wait on #23678, which is included in this to make the openssl version check succeed and to avoid conflicts because of adjacent test assertions. I'll rebase after the other is merged.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Oct 15, 2018
@sam-github sam-github force-pushed the test-all-process-version-strings branch from 8401ff8 to 429cb46 Compare October 15, 2018 23:15
@sam-github
Copy link
Contributor Author

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on the second commit.
(The first one got 👍 in #23678)

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails in the CI, but LGTM once it passes.

@richardlau
Copy link
Member

richardlau commented Oct 16, 2018

This fails in the CI,

For reference:

not ok 1451 parallel/test-process-versions

https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_withoutintl_x64/7845/console

19:38:16 not ok 1451 parallel/test-process-versions
19:38:16   ---
19:38:16   duration_ms: 0.616
19:38:16   severity: fail
19:38:16   exitcode: 1
19:38:16   stack: |-
19:38:16     assert.js:351
19:38:16         throw err;
19:38:16         ^
19:38:16     
19:38:16     AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
19:38:16     
19:38:16       assert(/^\d+\.\d+$/.test(process.versions.cldr))
19:38:16     
19:38:16         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_withoutintl_x64/test/parallel/test-process-versions.js:37:1)
19:38:16         at Module._compile (internal/modules/cjs/loader.js:707:30)
19:38:16         at Object.Module._extensions..js (internal/modules/cjs/loader.js:718:10)
19:38:16         at Module.load (internal/modules/cjs/loader.js:605:32)
19:38:16         at tryModuleLoad (internal/modules/cjs/loader.js:544:12)
19:38:16         at Function.Module._load (internal/modules/cjs/loader.js:536:3)
19:38:16         at Function.Module.runMain (internal/modules/cjs/loader.js:760:12)
19:38:16         at startup (internal/bootstrap/node.js:303:19)
19:38:16         at bootstrapNodeJSCore (internal/bootstrap/node.js:870:3)
19:38:16   ...

The version strings from ICU (process.versions.cldr, process.versions.icu and process.versions.unicode) should be guarded by common.hasIntl and process.versions.openssl test should be guarded by common.hasCrypto. I don't think the CI has a --without-ssl build.

@sam-github sam-github force-pushed the test-all-process-version-strings branch 3 times, most recently from 895db90 to b2ec97a Compare October 16, 2018 16:48
@sam-github sam-github force-pushed the test-all-process-version-strings branch 2 times, most recently from 93b5c7e to ba0fb46 Compare October 23, 2018 03:47
@sam-github
Copy link
Contributor Author

@sam-github sam-github force-pushed the test-all-process-version-strings branch 2 times, most recently from 97a88d2 to 43c451d Compare October 24, 2018 16:17
@sam-github
Copy link
Contributor Author

sam-github commented Oct 24, 2018

@jasnell
Copy link
Member

jasnell commented Oct 24, 2018

Many were checked, but a few were not.
@sam-github sam-github force-pushed the test-all-process-version-strings branch from 43c451d to 38c1e05 Compare October 24, 2018 21:34
@sam-github
Copy link
Contributor Author

versions.tz also needed protection for without-icu builds.

ci: https://ci.nodejs.org/job/node-test-pull-request/18128/

@sam-github
Copy link
Contributor Author

Landed in f4225f0

@sam-github
Copy link
Contributor Author

Landed in eb6b5c3

@sam-github sam-github closed this Oct 26, 2018
@sam-github sam-github deleted the test-all-process-version-strings branch October 26, 2018 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants