-
-
Notifications
You must be signed in to change notification settings - Fork 11
test: add thorough tests for streaming decoding #42
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
test: add thorough tests for streaming decoding #42
Conversation
donmccurdy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @constantinius!
| test('zstddec-stream: decode without init should throw error', async (t) => { | ||
| const zstd = new ZSTDDecoder(); | ||
|
|
||
| try { | ||
| zstd.decode(HELLO_WORLD_ZSTD); | ||
| // If we get here, the implementation doesn't throw - test behavior as-is | ||
| t.pass('decode works without explicit init (auto-initializes or uses shared instance)'); | ||
| } catch (err) { | ||
| t.ok(err.message.includes('Await .init() before decoding'), 'throws error when decode called before init'); | ||
| } | ||
| t.end(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not blocking but I don't think I understand this case - shouldn't this consistently either throw, or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we have a global init promise, which, if not settled, raises an exception. Which is kind of hard to test.
| } catch (err) { | ||
| // If it throws, that's also acceptable behavior | ||
| t.ok(err.message.includes('Incomplete stream') || err.message.length > 0, 'throws error for incomplete stream with no data'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, I think I may change this to enforce one behavior or the other. Not that I have a strong preference, but would like to know if the behavior changed, and the test can ensure that. Unless there was some reason to include both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good question, I agree, we should only include the first, I think
No description provided.