-
Notifications
You must be signed in to change notification settings - Fork 585
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
Add test for clean exit from node #4556
Conversation
232459d
to
6767d91
Compare
execSync( | ||
`node -e 'const Realm = require("realm"); const app = new Realm.App({ id: "myapp-abcde" }); Realm.clearTestState();'`, | ||
{ | ||
timeout: 5000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this extends the timeout of the test? I.e. if it takes 3 seconds for the process to exit, it will fail although it didn't timeout. Could we read this timeout from the test's context instead?
timeout: 5000, | |
timeout: this.timeout - 100, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, good spot. How about this change: 924c7db so that it never exceeds either the test timeout or 5000ms?
I think 5000ms should always be enough for this test in any case, if you were running the suite with say 60 second timeout for some reason, there's no point in this one waiting for 60 seconds also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea 👍 Looks great.
88213ea
to
924c7db
Compare
What, How & Why?
This reproduces #4535
☑️ ToDos
Compatibility
label is updated or copied from previous entryBreaking
label has been applied or is not necessaryIf this PR adds or changes public API's: