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

chore(test-utils): Update tests to use async device creation #2226

Merged
merged 6 commits into from
Sep 1, 2024

Conversation

ibgreen
Copy link
Collaborator

@ibgreen ibgreen commented Aug 25, 2024

For #

Background

  • For convenience the webglTestDevice export was created sync. This meant that Khronos WebGL tool were not dynamically loaded, which makes it harder to debug webgl errors.

Change List

  • Update all tests to use async device creation.
  • Remove sync test exports

Notes

  • Keep but deprecate the sync getTestDevice() and webglDevice exports from @luma.gl/test-utils for now until deck.gl has moved to async tests.

@ibgreen ibgreen changed the title chore: Update tests to use async device creation chore(test-utils): Update tests to use async device creation Aug 26, 2024
@@ -469,7 +485,9 @@ test('WebGLState#withGLParameters framebuffer', t => {
t.end();
});

test('WebGLState#withGLParameters empty parameters object', t => {
test('WebGLState#withGLParameters empty parameters object', async t => {
const webglDevice = await getWebGLTestDevice();
Copy link
Collaborator

Choose a reason for hiding this comment

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

A lot of repetition here. Would it be worth defining a helper function webGlTest() that passed in the created context?

webGlTest('WebGLState#withGLParameters empty parameters object', async (t, webglDevice) => {...})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A lot of repetition here. Would it be worth defining a helper function webGlTest() that passed in the created context?

webGlTest('WebGLState#withGLParameters empty parameters object', async (t, webglDevice) => {...})

That could be a good idea.

  • We'll be moving most of the WebGL specific tests (not this file of course) to multi-platform, so we will want to call getTestDevices() instead of getWebGLTestDevice(), but I suppose we could supply a testDevices param.
  • I'd like to move to jest and I wonder if there is not a beforeTest() type method that could wait for the device and store it in a global var.
  • I tend to be a bit wary of wrapping things, it makes the code base a bit less clear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One further disadvantage of the wrapper is that test.only etc stops working

@ibgreen ibgreen merged commit 304bdb5 into master Sep 1, 2024
1 check passed
@ibgreen ibgreen deleted the ib/async-device-tests branch September 1, 2024 21:29
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