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

Fix broken tests on main #165

Merged
merged 1 commit into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"javascript.validate.enable": true,
"editor.codeActionsOnSave": {
"source.fixAll.eslint": "explicit"
},
"editor.formatOnSave": true
}
3 changes: 3 additions & 0 deletions tests/fixtures/batch-test-group/.bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Disable Bzlmod
common --noenable_bzlmod
common --enable_workspace
1 change: 1 addition & 0 deletions tests/fixtures/batch-test-group/.bazelversion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
7.1.2
3 changes: 3 additions & 0 deletions tests/fixtures/bazel-dependent-builds/.bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Disable Bzlmod
common --noenable_bzlmod
common --enable_workspace
1 change: 1 addition & 0 deletions tests/fixtures/bazel-dependent-builds/.bazelversion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
7.1.2
3 changes: 3 additions & 0 deletions tests/fixtures/bazel-dependent-failure/.bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Disable Bzlmod
common --noenable_bzlmod
common --enable_workspace
1 change: 1 addition & 0 deletions tests/fixtures/bazel-dependent-failure/.bazelversion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
7.1.2
3 changes: 3 additions & 0 deletions tests/fixtures/bazel-rules/.bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Disable Bzlmod
common --noenable_bzlmod
common --enable_workspace
1 change: 1 addition & 0 deletions tests/fixtures/bazel-rules/.bazelversion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
7.1.2
3 changes: 3 additions & 0 deletions tests/fixtures/bin/.bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Disable Bzlmod
common --noenable_bzlmod
common --enable_workspace
1 change: 1 addition & 0 deletions tests/fixtures/bin/.bazelversion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
7.1.2
3 changes: 3 additions & 0 deletions tests/fixtures/commands/.bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Disable Bzlmod
common --noenable_bzlmod
common --enable_workspace
1 change: 1 addition & 0 deletions tests/fixtures/commands/.bazelversion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
7.1.2
3 changes: 3 additions & 0 deletions tests/fixtures/find-changed-targets/bazel/.bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Disable Bzlmod
common --noenable_bzlmod
common --enable_workspace
1 change: 1 addition & 0 deletions tests/fixtures/find-changed-targets/bazel/.bazelversion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
7.1.2
3 changes: 3 additions & 0 deletions tests/fixtures/report-mismatched-top-level-deps/.bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Disable Bzlmod
common --noenable_bzlmod
common --enable_workspace
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
7.1.2
3 changes: 3 additions & 0 deletions tests/fixtures/scaffold/.bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Disable Bzlmod
common --noenable_bzlmod
common --enable_workspace
1 change: 1 addition & 0 deletions tests/fixtures/scaffold/.bazelversion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
7.1.2
3 changes: 3 additions & 0 deletions tests/fixtures/script/.bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Disable Bzlmod
common --noenable_bzlmod
common --enable_workspace
1 change: 1 addition & 0 deletions tests/fixtures/script/.bazelversion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
7.1.2
3 changes: 3 additions & 0 deletions tests/fixtures/yarn-pnp/.bazelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Disable Bzlmod
common --noenable_bzlmod
common --enable_workspace
1 change: 1 addition & 0 deletions tests/fixtures/yarn-pnp/.bazelversion
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
7.1.2
77 changes: 67 additions & 10 deletions tests/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,10 @@ const {shouldSync, getVersion} = require('../utils/version-onboarding.js');
const yarnCmds = require('../utils/yarn-commands.js');
const {sortPackageJson} = require('../utils/sort-package-json');

const originalProcessExit = process.exit;
process.on('unhandledRejection', e => {
console.error(e.stack);
process.exit(1);
originalProcessExit(1);
});

// $FlowFixMe flow can't handle statics of async function
Expand All @@ -80,32 +81,88 @@ const tmp = tmpdir();
runTests();

async function t(test) {
const testName = test.name;
const match = (process.argv[2] || '').toLowerCase();
if (test.name.toLowerCase().indexOf(match) > -1) {

if (testName.toLowerCase().indexOf(match) > -1) {
if (match) console.log(`Testing ${test.name}`);
try {
await test();
console.log(`✅ ${testName}`);
} catch (e) {
console.error(`\n\nTest "${test.name}" failed with error:`);
console.log(`❌ ${testName}`);
throw e;
}
}
}
Comment on lines 83 to 97
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just provides some visual differentiation in the logs


/**
* Uses an error stack trace to determine which test from this
* file caused a given function to be called.
*/
function getCallingTestFn() {
const error = new Error();
if (!error.stack) return null;

const errorStackLines = error.stack.split('\n');
for (const line of errorStackLines) {
const trimmedLine = line.split('at ')[1];
if (!trimmedLine) continue;

const [, fnName, filePath] =
trimmedLine.match(
/^(?:async )?(?:(.+) \()?(?:file:\/\/)?([^:]+):(\d+):(\d+)\)?$/
) || [];

if (
filePath === __filename &&
fnName.startsWith('test') &&
fnName.length > 4
) {
return fnName;
}
}

return null;
}

const testsExpectingExit /*: Map<string, number> */ = new Map();
// $FlowFixMe
process.exit = (code = 0) => {
const callingTest = getCallingTestFn();
if (callingTest && testsExpectingExit.has(callingTest)) {
testsExpectingExit.set(callingTest, code);
return;
}

// $FlowFixMe: bad libdef
assert.fail(`'process.exit' was unexpectedly called with code '${code}'`);
};
Comment on lines +129 to +140
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a number of tests that use expectProcessExit, but only on commands they're expecting to fail. When commands unexpectedly fail, a lot of them will directly call process.exit via spawnOrExit:

const spawnOrExit /*: Spawn */ = async (...args) => {
try {
return await spawn(...args);
} catch (e) {
process.exit(e.status || 1);
}
};

This causes the tests process to immediately exit without reporting which test failed or showing any stack traces.

The mechanism I added here allows expectProcessExit to behave exactly as it did before, but also detects unexpected calls of process.exit and instead throws an assertion error, scoping it to the call stack of the test that caused it, and allowing the test runner to properly report the failure. This makes debugging tests much easier.

The new getCallingTestFn (a little hacky, but works reliably) is used because a lot of tests run in parallel, so we need to be able to identify which test caused process.exit to be called, and only suppress the ones that are expected via expectProcessExit.

You can see how helpful this new mechanism is when the testBazelBuild test fails. Previously the test process exited without any helpful reporting, but with the new mechanism it's very clear what failed and where:

before
before

after
after


async function expectProcessExit(expectedExitCode, run) {
const callingTest = getCallingTestFn() || '';
const shouldUseFallbackExitMock = !callingTest;

let exitCode = 0;
// $FlowFixMe
const originalExit = process.exit;
// $FlowFixMe
process.exit = (code = 0) => {
exitCode = code;
};
if (shouldUseFallbackExitMock) {
// $FlowFixMe
process.exit = (code = 0) => {
exitCode = code;
};
} else {
testsExpectingExit.set(callingTest, 0);
}

try {
await run();
} finally {
// $FlowFixMe
process.exit = originalExit;
if (shouldUseFallbackExitMock) {
// $FlowFixMe
process.exit = originalExit;
} else {
exitCode = testsExpectingExit.get(callingTest) || 0;
}

assert.strictEqual(
exitCode,
Expand Down
Loading