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

Fix async portions of PLS test. #3562

Merged
merged 1 commit into from
Jul 6, 2023
Merged

Conversation

kenrussell
Copy link
Member

Await the results of asynchronous functions, and explicitly call finishTest() at the end of runTest().

Follow-on to #3530 .

Await the results of asynchronous functions, and explicitly call
finishTest() at the end of runTest().

Follow-on to KhronosGroup#3530 .
@kenrussell
Copy link
Member Author

@csmartdalton please feel free to review too.

Copy link
Contributor

@csmartdalton csmartdalton left a comment

Choose a reason for hiding this comment

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

Thank you for putting this together Ken!

await checkRendering(gl);
await checkRendering(gl_no_alpha);

finishTest();
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my own understanding, how does the test harness know that the test is async now and that it should wait for finishTest? Before was it assuming the test was over as soon as it finished parsing?

Copy link
Member

Choose a reason for hiding this comment

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

js-test-post.js notifies the test harness explicitly. When included statically, it's called as soon as the parsing is finished. With finishTest, the same script is added on the fly.

@kenrussell kenrussell merged commit 795e62c into KhronosGroup:main Jul 6, 2023
2 checks passed
@kenrussell kenrussell deleted the fix-pls branch July 6, 2023 20:29
@lexaknyazev
Copy link
Member

@kenrussell Please add another finishTest() call when the extension is not supported to avoid test timeout.

kenrussell added a commit to kenrussell/WebGL that referenced this pull request Jul 6, 2023
Timeouts were accidentally introduced in the last commit by failing to
do this. Follow-on to KhronosGroup#3562 and KhronosGroup#3530.
@kenrussell
Copy link
Member Author

@kenrussell Please add another finishTest() call when the extension is not supported to avoid test timeout.

Thanks for catching that. Fixing in #3563 .

kenrussell added a commit that referenced this pull request Jul 6, 2023
* Call finishTest() when PLS isn't supported.

Timeouts were accidentally introduced in the last commit by failing to
do this. Follow-on to #3562 and #3530.

Fixed bugs caught by @lexaknyazev in code review.
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.

None yet

3 participants