Skip to content

Fix flaky resolveNetworkAddress test#25249

Merged
ravicious merged 3 commits intomasterfrom
ravicious/resolve-addr-test
May 8, 2023
Merged

Fix flaky resolveNetworkAddress test#25249
ravicious merged 3 commits intomasterfrom
ravicious/resolve-addr-test

Conversation

@ravicious
Copy link
Copy Markdown
Member

I had the resolveNetworkAddress test fail randomly. resolveNetworkAddress failed with the following error:

Could not resolve address (unix:///tmp/test) for process /opt/hostedtoolcache/node/16.20.0/x64/bin/node: the process exited.

There are two problems here.

Truthiness check

The function which returns this error is called on the exit handler of a child process:

const rejectOnExit = (code: number, signal: NodeJS.Signals) => {
const codeOrSignal = [
code && `code ${code}`,
signal && `signal ${signal}`,
]
.filter(Boolean)
.join(' and ');
const details = codeOrSignal ? ` with ${codeOrSignal}` : '';
rejectOnError(
new ResolveError(
requestedAddress,
process,
`the process exited${details}`
)
);
};

Node.js docs say that either the code or the signal will be non-null, so why is neither of them present in the error message? Well, that's because code is a number and if it's 0, meaning the process exited without errors, then it's going to evaluate to false.

Race condition

The second problem: how did the child process exit without errors yet the test failed? The test spawns a child process running testProcess.js and then waits for the process to write {CONNECT_GRPC_PORT: 1337} to its stdout.

If the child process exit handler was triggered before the stdout data handler, then it must mean that testProcess.js exited before writing that message to stdout.

This is how testProcess.js looks like:

const process = require('process');
const sleep = ms => new Promise(resolve => setTimeout(resolve, ms));
(async () => {
const waitTime = parseInt(process.argv[2]);
if (waitTime) {
await sleep(waitTime);
}
const shouldExit = process.argv[3];
if (shouldExit) {
process.exit(1);
}
console.log('Lorem ipsum dolor sit amet');
console.log('{CONNECT_GRPC_PORT: 1337}');
console.log('Lorem ipsum dolor sit amet');
})();

I realized that there's nothing awaiting this anonymous async function, so it's totally possible for the process to just not write anything to stdout. Since top-level await is available in .mjs modules since Node v14, I rewrote the file to use it. This guarantees that the process will write to stdout before exiting.

Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

Converting testProcess.js to ES modules is cool!

const codeOrSignal = [
code && `code ${code}`,
// code can be 0, so we cannot just check it the same way as the signal.
code != null && `code ${code}`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we be more strict? (code !== null)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is one of the cases where it covers both null and undefined. I want to perform a truthiness check on a number without treating 0 as falsy.

@ravicious ravicious added this pull request to the merge queue May 8, 2023
Merged via the queue into master with commit 273d1d9 May 8, 2023
@ravicious ravicious deleted the ravicious/resolve-addr-test branch May 8, 2023 09:51
@public-teleport-github-review-bot
Copy link
Copy Markdown

@ravicious See the table below for backport results.

Branch Result
branch/v10 Create PR
branch/v11 Create PR
branch/v12 Create PR
branch/v13 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants