doc: update tests to avoid running in parallel

[mhdawson]

fixes: #1022

The objectwrap_worker_thread and symbol tests
were not waiting for the test to complete before the
subsequent tests were started. This caused intermittent
crashes in the CI.

Updated both tests so that they complete before the
next test runs.

Signed-off-by: Michael Dawson [email protected]

[mhdawson]

@nodejs/node-api anybody have a windows machine who can check what's up with the symbol test on windows? I'm not sure how the change would have broken just for windows?

[NickNaso]

Hi @mhdawson,
it seems that when we call the function assertSymbolIsWellknown (https://github.com/nodejs/node-addon-api/blob/main/test/symbol.js#L30) for the symbol matchAll we get undefined and the test fail.
If I do the same in native JavaScript:

'use strict'
const assert = require('assert')

const symbols = ['iterator', 'matchAll']

for (const symbol of symbols) {
    const symbOne = Symbol.for(symbol);
    const symbTwo = Symbol.for(symbol);
    assert(symbOne && symbTwo);
    assert(symbOne === symbTwo);
}

for (const symbol of symbols) {
    const symbOne = global.Symbol[symbol]
    const symbTwo = global.Symbol[symbol]
    assert(symbOne && symbTwo);
    assert(symbOne === symbTwo);
}

In the example above the second series of test fails. This happens becasue Napi::Symbol::Wellknown is implemented like reported below:

inline Symbol Symbol::WellKnown(napi_env env, const std::string& name) {
  return Napi::Env(env).Global().Get("Symbol").As<Object>().Get(name).As<Symbol>();
}

To obtain a well-known symbols we try to get it from the global object.
In Node.js 10.x and Windows if you try to get the matchAll symbol through a global object global.Symbol.matchAll you will get undefined.
The MSDN documentation reports that matchAll symbol is fully supported on Node.js 12.0.0 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/matchAll#browser_compatibility

[mhdawson]

@NickNaso thanks! Strange that it is found on linux but not windows.

[legendecas]

The above two helpers are named with their parameters:

• runTest(test: (binding) => Promise<void>)
• runTestWithBindingPath(test: (bindingPath) => Promise<void>)

Maybe we can name this as runTestWithBuildType so that we can assume the first parameter of the test function is buildType?

[mhdawson]

@legendecas that makes sense to me will update.

[mhdawson]

@NickNaso this is what I see on linux

[midawson@drx-hemera node]$ cat test.js
console.log(global.Symbol.matchAll);
console.log(global.Symbol['matchAll']);
[midawson@drx-hemera node]$ ./node test.js
Symbol(Symbol.matchAll)
Symbol(Symbol.matchAll)

From what I understand you get undefined on windows instead, right ?

[mhdawson]

@NickNaso, I thinkI understand you now, sounds like we should only test for that symbol on 12.x and above

[NickNaso]

@NickNaso this is what I see on linux

[midawson@drx-hemera node]$ cat test.js
console.log(global.Symbol.matchAll);
console.log(global.Symbol['matchAll']);
[midawson@drx-hemera node]$ ./node test.js
Symbol(Symbol.matchAll)
Symbol(Symbol.matchAll)

From what I understand you get undefined on windows instead, right ?

Yes on Windows, but only with Node.js 10.x I get undefined.

[mhdawson]

Landed as a459f5c