-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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 Unhandled promise rejection: undefined thrown
in CI
#12267
Conversation
Thank you for the pull request, @ggetz! ✅ We can confirm we have a CLA on file for you. |
I ran the tests twice and got the same 8 test failures each time. Are these related? I do not remember seeing these before.
|
These are unrelated. The lighting specs were added in #12129, and I can loosen the epsilon checks. I'll open a new PR and tag you for review. |
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.
no longer seeing the errors 👍
Basically all the tests are using the pattern
This was changed for one case now (namely, the one before the one that was
I wonder whether it could make sense to systematically and mechanically chage the pattern from above into
? |
I assume you are talking about the change from a promise chain to async/await? This was changed because async/await is more readable than nested promise chains, and we'd like to opt for it where we can. Since the codebase has MANY instances of promise chains throughout since we were using promises far before async/await syntax was available for use, I'd be hesitant to add linting for it right now, but eventually we might want to add a rule like I don't think this changes the behavior of the test or prevents race conditions, other than just making things more readable and hopefully less error prone. Async functions implicitly always return a promise. |
This refers to some guesses about where race conditions come from. And that's difficult. (I knew that since I wrote my first multi-threaded program that just printed But the thought was revolving about a pattern like this:
Now, how does Jasmine execute this? The tests are not If both the tests contained the Wild guesses, sure but ... it's not totally unreasonable, is it? |
If a spec returns a Promise, like They also have a bunch of async related answers in the FAQ https://jasmine.github.io/pages/faq.html#async
As Gabby stated:
As long as they return a promise they should be treated the same way as writing it with |
OK, from quickly looking over this, the part at https://jasmine.github.io/pages/faq.html#late-failures , seems to ...
And about the statement from the tutorial page:
That return is obviously important. I think that this is on the radar, and it may be unlikely to be a widespread flaw, but ... one instance of this flaw would be enough. So one review pass for the current specs could be to carefully go though each of them, and see whether there is any case where the code is
or
instead of
(An example that was intentionally chosen to show how subtle that missing |
Description
I think I caught the race condition: Basically whenever the camera moved or we started rendering a tileset, there were tiles that started load. When the test ended, the tileset was destroyed, and the error was thrown. So we'll need to await the tiles loaded helper to ensure these test pass.
Issue number and link
Fixes #11958
Testing plan
CI should pass, even if you re-run!
Author checklist
CONTRIBUTORS.md
I have updatedCHANGES.md
with a short summary of my changeI have updated the inline documentation, and included code examples where relevant