-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
ci: optimize ParseLiveQuery.spec.js timeouts #7671
ci: optimize ParseLiveQuery.spec.js timeouts #7671
Conversation
Thanks for opening this pull request!
|
@mtrezza, after my last comment in the earlier PR I remained courious how much this could be further optimised. Hope this helps... (main change is the reduced tmo in |
Codecov Report
@@ Coverage Diff @@
## release-4.x.x #7671 +/- ##
================================================
Coverage ? 93.21%
================================================
Files ? 169
Lines ? 12442
Branches ? 0
================================================
Hits ? 11598
Misses ? 844
Partials ? 0 Continue to review full report at Codecov.
|
spec/ParseLiveQuery.spec.js
Outdated
@@ -5,6 +5,8 @@ const validatorFail = () => { | |||
throw 'you are not authorized'; | |||
}; | |||
|
|||
console.log('jasmine.ASYNC_TEST_WAIT_TIME=' + jasmine.ASYNC_TEST_WAIT_TIME); |
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.
this needs to be removed...
await obj2.save(); | ||
await new Promise(resolve => setTimeout(resolve, 200)); | ||
await new Promise(resolve => setTimeout(resolve, 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.
tried with 10msec but the test fails
if (current.get('foo') != original.get('foo')) { | ||
req.sendEvent = false; | ||
} | ||
setTimeout(done, 1000); |
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.
unclear why this is 1000msec, reducing will probably not cause the test to fail but might falsely have it succeed..
setTimeout(async () => { | ||
done(); | ||
}, 1000); | ||
setTimeout(done, 1000); |
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.
unclear why this is 1000msec, reducing will probably not cause the test to fail but might falsely have it succeed..
spec/ParseUser.spec.js
Outdated
subscription.unsubscribe(); | ||
const client = await Parse.CoreManager.getLiveQueryController().getDefaultLiveQueryClient(); | ||
client.close(); | ||
await new Promise(resolve => setTimeout(resolve, 10)); |
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.
this is the key change, was 1000msec
Hi @dblythy, would you mind having a quick look at this? Anything in here that might negatively change the outcome of a test (especially false positives)? |
3a46302
to
be67738
Compare
I haven't looked at the individual changes, but generally I'd say:
While this PR focuses on reducing the individual test run, there is another optimization perspective which is reducing the overall compute time. These two are currently the same, but they don't have to be. I opened #7672 for details. |
Closing as the base branch of this PR |
New Pull Request Checklist
Issue Description
Potential unnecessary long timeouts for the test.
Related issue: #7659
Approach
Reduced timeouts resulting in 5x speedup.
TODOs before merging