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: migrate from tape to tap #1795

Closed
wants to merge 8 commits into from
Closed

test: migrate from tape to tap #1795

wants to merge 8 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jun 22, 2019

alternative to #1171

Needs to be tested against CI. If we're not using tap output in CI I'd like to remove -Rtap.

@rvagg
Copy link
Member Author

rvagg commented Jun 22, 2019

@cclauss I probably should have run your Python changes through CI because there are some problems on Windows which may be coming from it? See https://ci.nodejs.org/job/nodegyp-test-commit/nodes=win2016-vs2017/678/console

15:29:52     # Subtest: build simple addon in path with non-ascii characters
15:29:52         1..1
15:29:52 Traceback (most recent call last):
15:29:52   File "fixtures/test-charmap.py", line 23, in <module>
15:29:52     print(main())
15:29:52   File "fixtures/test-charmap.py", line 19, in main
15:29:52     print(textmap[encoding])
15:29:52   File "C:\Python27\lib\encodings\cp1252.py", line 12, in encode
15:29:52     return codecs.charmap_encode(input,errors,encoding_table)
15:29:52 UnicodeEncodeError: 'charmap' codec can't encode character u'\u012b' in position 3: character maps to <undefined>
15:29:52         ok 1 - python console app can't encode non-ascii character. # SKIP
15:29:52         # skip: 1
15:29:52     ok 3 - build simple addon in path with non-ascii characters # time=67.458ms
15:29:52     

Could that be coming from abef93d which did:

-  if textmap.has_key(encoding):
-    print textmap[encoding]
+  if encoding in textmap:
+    print(textmap[encoding])

@rvagg
Copy link
Member Author

rvagg commented Jun 22, 2019

CI for this: https://ci.nodejs.org/view/All/job/nodegyp-test-pull-request/133/
Failing on Windows with the python problem above, but not failing on Node 12, I'm not sure what that's about. Some funky unicode difference that's coming down in from the test suite?

CI isn't using tap output at all for anything automatic, but it is handy for text output. So what I've done is introduce a "test-ci" script that will be run on Travis (and I'll update Jenkins to use it too), otherwise it'll do the normal prettified tap output.

@cclauss
Copy link
Contributor

cclauss commented Jun 22, 2019

Our current Travis CI runs on Linux but we can easily add Windows and/or macOS if that would be useful.

@rvagg
Copy link
Member Author

rvagg commented Jun 22, 2019

very useful, so many of our problems are OS specific here.

@rvagg
Copy link
Member Author

rvagg commented Jun 22, 2019

A bunch more minor updates to make the standard npm test output nice and clean, so turning off logging output. If you have your loglevel set to something custom it's going to make the tests noisier, but there's also some stuff in there that will log.warn and some console.log too.

There's a new deprecation in Node 12 that spits out to stderr because we're using a self-signed cert with an IP address, so there's a commit in here that replaces that with a self-signed for localhost and the warning goes away.

Test output:

Screenshot 2019-06-22 16 30 04

npm run test-ci is the verbose nested TAP output (but without the stderr mess) plus the coverage at the bottom as well.

@richardlau
Copy link
Member

CI isn't using tap output at all for anything automatic, but it is handy for text output. So what I've done is introduce a "test-ci" script that will be run on Travis (and I'll update Jenkins to use it too), otherwise it'll do the normal prettified tap output.

Travis is probably better off with the prettified output. As it is it doesn't display all of it (output is too long):
image

For the CI the only reason I can think of to keep the tap output is if we want to use tap2junit on it (which we don't currently).

Since switching to tap would by default enable coverage we could consider enabling codecov (which we use elsewhere in the org, e.g. citgm https://codecov.io/gh/nodejs/citgm, node-core-utils https://codecov.io/gh/nodejs/node-core-utils) for this repository.

@rvagg
Copy link
Member Author

rvagg commented Jun 25, 2019

👍 you're right, it looks pretty good in Travis: https://travis-ci.com/nodejs/node-gyp/jobs/210631751

rvagg added a commit that referenced this pull request Jun 25, 2019
PR-URL: #1795
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
rvagg added a commit that referenced this pull request Jun 25, 2019
PR-URL: #1795
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@rvagg rvagg closed this Jun 25, 2019
@rvagg rvagg deleted the rvagg/tap branch June 25, 2019 01:27
@rvagg
Copy link
Member Author

rvagg commented Jun 25, 2019

landed in 395f843

@rvagg rvagg mentioned this pull request Jun 26, 2019
@joaocgreis
Copy link
Member

This broke Node v6. CI failed because of the python issues, and that probably caused the real failures in v6 to go unnoticed.

What should we do here? I don't think we should support unsupported Node versions at all, but for that we need to update the engines field and exclude from CI. Or should we revert this and keep supporting Node v6? It would probably be nicer to drop support on a semver-major version, though I'm not sure we strictly need to.

cc @nodejs/node-gyp

@cclauss
Copy link
Contributor

cclauss commented Jul 4, 2019

CI failed because of the python issues

Can you please provide a URL to these failures?

@joaocgreis
Copy link
Member

https://ci.nodejs.org/job/nodegyp-test-commit/676/ and https://ci.nodejs.org/job/nodegyp-test-commit/677/

There are two failures. On CI, every run of v6.2.1 and v6.14.4 failed while running npm install:

06:31:49 npm ERR! enoent ENOENT: no such file or directory, rename '/home/iojs/build/workspace/nodegyp-test-commit/nodes/ubuntu1604-64/node_modules/.staging/@types/prop-types-5c4fdf37' -> '/home/iojs/build/workspace/nodegyp-test-commit/nodes/ubuntu1604-64/node_modules/tap/node_modules/@types/prop-types'
06:31:49 npm ERR! enoent ENOENT: no such file or directory, rename '/home/iojs/build/workspace/nodegyp-test-commit/nodes/ubuntu1604-64/node_modules/.staging/@types/prop-types-5c4fdf37' -> '/home/iojs/build/workspace/nodegyp-test-commit/nodes/ubuntu1604-64/node_modules/tap/node_modules/@types/prop-types'

Locally, if I use a newer version of npm, this does not happen. This is probably an issue with npm or the module being installed, I don't know if it is related to this PR. If we're to support Node v6 we should work around this somehow if possible.

However, after using a new npm to install, this error happens when running npm test:

C:\Users\Administrator\Desktop\node-gyp\node_modules\tap\bin\run.js:125
const main = async options => {
                   ^^^^^^^

SyntaxError: Unexpected identifier
    at createScript (vm.js:56:10)
    at Object.runInThisContext (vm.js:97:10)
    at Module._compile (module.js:549:28)
    at Object.Module._extensions..js (module.js:586:10)
    at Module.load (module.js:494:32)
    at tryModuleLoad (module.js:453:12)
    at Function.Module._load (module.js:445:3)
    at Module.runMain (module.js:611:10)
    at run (bootstrap_node.js:394:7)
    at startup (bootstrap_node.js:160:9)
npm ERR! Test failed.  See above for more details.

@rvagg
Copy link
Member Author

rvagg commented Jul 5, 2019

addressing it in #1808, but maybe we need to extend our travis matrix to include the major Node versions, probably just on Linux (I don't know which version of Python, maybe it doesn't matter).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants