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

shard realm server tests #2015

Merged
merged 9 commits into from
Jan 8, 2025
Merged

shard realm server tests #2015

merged 9 commits into from
Jan 8, 2025

Conversation

habdelra
Copy link
Contributor

@habdelra habdelra commented Jan 7, 2025

make sure to hide whitespace when reviewing this PR. the lines changed is super deceptive as it's 99.9% whitespace. (there is also a lot of prettier updates caught up in this PR as someone doesn't have that enabled 😑...)

Copy link

github-actions bot commented Jan 7, 2025

Host Test Results

    1 files  ±0      1 suites  ±0   21m 46s ⏱️ +27s
725 tests ±0  723 ✔️ ±0  2 💤 ±0  0 ±0 
730 runs  ±0  728 ✔️ ±0  2 💤 ±0  0 ±0 

Results for commit 83e0cc5. ± Comparison against base commit 8dde0cd.

♻️ This comment has been updated with latest results.

@habdelra habdelra marked this pull request as ready for review January 7, 2025 21:40
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 script is used to make sure that the ci.yaml doesnt forget to be updated when a new test module is added


function createJWT(expiresIn: string | number) {
return jwt.sign({}, 'secret', { expiresIn });
}

module('realm-auth-client', function (assert) {
let client: RealmAuthClient;
module(basename(__filename), function () {
Copy link
Contributor Author

@habdelra habdelra Jan 7, 2025

Choose a reason for hiding this comment

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

this is a tiny little boilerplate that we need to add to each test file made sure we can correlate filenames to test modules so we can filter them properly. this diff is a little hard to read without hiding white space, but this is an additive change--just wrapping the entire test module with this call.

@habdelra habdelra requested a review from a team January 7, 2025 21:43
@habdelra
Copy link
Contributor Author

habdelra commented Jan 7, 2025

So a general observation here. sharding the server tests doens't actually make things run faster. it looks like for all the server tests they finish pretty quickly, but after the tests runto completion and the test results are printed out, there is still some timer(s) that seems to be keeping the node process from exiting promptly. I'm guessing it's in some lib code that we are using.

@habdelra
Copy link
Contributor Author

habdelra commented Jan 7, 2025

perhaps we can write our own custom test reporter that wraps the existing reporter and just calls a process.exit() to force the test to end (i noticed that process.exit() does actually work to force the process to end)

Comment on lines 16 to 40
// There is some timer that is preventing the node process from ending promptly.
// This forces the test to end with the correct response code. Note than a
// message "Error: Process exited before tests finished running" will be
// displayed because of this approach.
import QUnit from 'qunit';
(QUnit as any).on(
'runEnd',
({
testCounts,
}: {
testCounts: {
passed: number;
failed: number;
total: number;
skipped: number;
todo: number;
};
}) => {
if (testCounts.failed > 0) {
process.exit(1);
} else {
process.exit(0);
}
},
);
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 forces our tests to exit promptly despite some timer that seems to be preventing the node process from exiting

@habdelra
Copy link
Contributor Author

habdelra commented Jan 7, 2025

note the improved speeds of the realm server tests in the test results below (lol, it was the matrix tests that were slow this time)

Copy link
Contributor

@backspace backspace left a comment

Choose a reason for hiding this comment

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

a lot of prettier updates caught up in this PR as someone doesn't have that enabled

irksome, I sometimes save files without autoformatting because I don’t want spurious diffs 😖

@@ -12,3 +12,29 @@ import './queue-test';
import './realm-server-test';
import './virtual-network-test';
import './billing-test';

// There is some timer that is preventing the node process from ending promptly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it feasible we could fix the underlying problem vs having to forcefully exit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I tried to find the source of the issue but it was very mysterious. Just pausing a debugger after the test finishes but before the process ends never breaks. It doesn’t look like something obvious we are doing in the test, and I see it happening for really simple test modules too. It makes me feel like some lib that we are loading is doing this as a side effect. But yeah, I’m unable to figure out where it’s coming from.

packages/realm-server/scripts/lint-test-shards.ts Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@habdelra
Copy link
Contributor Author

habdelra commented Jan 8, 2025

a lot of prettier updates caught up in this PR as someone doesn't have that enabled

irksome, I sometimes save files without autoformatting because I don’t want spurious diffs 😖

I think it’s better to have consistent code formatting than spurious diffs. It makes things like find and replace much more predictable (like favoring ‘ vs “).

@habdelra habdelra merged commit ca1657f into main Jan 8, 2025
50 checks passed
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.

3 participants