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

Add tests for zstd content-encoding #44393

Merged
merged 3 commits into from
Feb 7, 2024

Conversation

nidhijaju
Copy link
Member

@nidhijaju nidhijaju commented Feb 5, 2024

This PR adds WPTs for decompressing different types of zstd-encoded content: simple, big, invalid, and an incompatibly large window size. This also reorganizes the tests so they are separated by compression scheme.

Copy link
Contributor

@ricea ricea left a comment

Choose a reason for hiding this comment

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

lgtm, nice.

@nidhijaju
Copy link
Member Author

@annevk Could you please take a look to see if these WPTs look reasonable? Thank you!

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This looks like reasonable zstd coverage. Although perhaps there should be more payloads? Does this also test a variety of invalid payloads and ensure the error handling is correct?

The other thing that is ideally tested is Content-Encoding handling. When it contains multiple values, strange whitespace characters, etc. That can be separate from this PR however.

@nidhijaju
Copy link
Member Author

Does this also test a variety of invalid payloads and ensure the error handling is correct?

I added some tests for invalid payloads similar to the ones for gzip. Please take a look.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks!

I think at some point we might want more exhaustive zstd coverage. Or is there another test suite elsewhere that implementations use?

I also still think we need more Content-Encoding coverage, but it doesn't have to all be in this PR.

@nidhijaju
Copy link
Member Author

Thank you for the review!

is there another test suite elsewhere that implementations use?

We have a set of unit tests in Chromium that we use to test the zstd decompression. Some of them can potentially be converted to WPTs to increase coverage in the future.

@nidhijaju nidhijaju merged commit 2453380 into web-platform-tests:master Feb 7, 2024
19 checks passed
@nidhijaju nidhijaju deleted the zstd-tests branch February 7, 2024 07:27
mbrodesser-Igalia pushed a commit to mbrodesser-Igalia/wpt that referenced this pull request Feb 19, 2024
* add zstd tests and separate into folders

* add bad input tests and make window size test tentative

* add https to bad input test
marcoscaceres pushed a commit that referenced this pull request Feb 23, 2024
* add zstd tests and separate into folders

* add bad input tests and make window size test tentative

* add https to bad input test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants