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

[core] Restore tests for CommandBuffer#copyTextureToBuffer #1868

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Dec 7, 2023

A few small but important fixes for CommandBuffer#copyTextureToBuffer, and restoring unit tests on that function. This PR is currently based on #1864, I'll hold it open until that is merged.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Solid work indeed! These tests clearly took some time to update but I suspect they will save us a lot of grief down the road.

@donmccurdy donmccurdy force-pushed the donmccurdy/remove-BufferWithAccessor branch from dbb7c56 to 52f690a Compare December 8, 2023 20:01
Base automatically changed from donmccurdy/remove-BufferWithAccessor to master December 8, 2023 20:12
@donmccurdy donmccurdy force-pushed the donmccurdy/command-buffer-tests branch from fc90ddc to 13d154d Compare December 8, 2023 20:16
Remove use of and changes to TEXTURE_FORMATS
@donmccurdy donmccurdy force-pushed the donmccurdy/command-buffer-tests branch from 13d154d to c229b9f Compare December 8, 2023 20:28
@donmccurdy donmccurdy merged commit 0ffdf31 into master Dec 8, 2023
1 of 2 checks passed
@donmccurdy donmccurdy deleted the donmccurdy/command-buffer-tests branch December 8, 2023 20:35
@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Dec 11, 2023

@ibgreen I noticed that this PR was also marked as a net loss in coverage, despite only adding tests:

Perhaps that's mostly explained by running tests in Node.js and without WebGL 2, as these new tests are skipped for coverage. But it is unexpected that Coveralls calls out uncovered lines in test files under modules/core-tests/test/**/*.spec.ts files. I wonder if that's an issue in how total coverage is computed.

@ibgreen
Copy link
Collaborator

ibgreen commented Dec 11, 2023

Thanks for digging into this.

Check out https://github.com/visgl/luma.gl/blob/master/nyc.config.js. We probably need to add core-tests to the excludes there.

@donmccurdy donmccurdy added this to the 9.0.0 milestone Feb 26, 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