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

Handle errors in ApproximateTerrainHeight.initialize #11818

Closed
wants to merge 1 commit into from

Conversation

jjhembd
Copy link
Contributor

@jjhembd jjhembd commented Feb 8, 2024

Description

This PR catches errors from ApproximateTerrainHeight.initialize and throws them as DeveloperErrors. This is to avoid erratic behavior when the promise is rejected.

When running tests locally on the webgl stub (npm test -- --webgl-stub), many runs would end early with an unclear error message:

  An error was thrown in afterAll
  Unhandled promise rejection: undefined thrown

Handling the error resolves the afterAll error in some cases.

Along the way, I cleaned up some references to obsolete .initialized flags.

Issue number and link

Possibly related to #11231.

Testing plan

Run tests locally with the webgl stub (npm test -- --webgl-stub). On my machine, I can consistently trigger the afterAll error in the #d1ed5ba commit from the voxel-pick-api branch. The changes in this PR consistently prevent the error.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • [ ] I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

.then(function (json) {
ApproximateTerrainHeights._terrainHeights = json;
})
.catch(function (error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So just to clarify, the promise was being rejected, but the returned value is undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what promise was rejected, or where undefined was thrown. ApproximateTerrainHeights was simply a guess, since it adds global state to so many test suites.

What is not clear to me is:

  • Why throwing the error solves the problem
  • Why the thrown error is not reported in the browser

Copy link
Contributor

@ggetz ggetz Feb 8, 2024

Choose a reason for hiding this comment

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

The error may be reported in the browser in the console. Have you checked?

@ggetz
Copy link
Contributor

ggetz commented Feb 9, 2024

Closing this. We discussed offline that this approach is covering up a deeper issue. Thanks for taking a look @jjhembd.

@ggetz ggetz closed this Feb 9, 2024
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.

2 participants