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

test: added payload too large test #24021

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MarioJGMsoft
Copy link
Contributor

Description

In OpCompressor.ts there is a serializeBatchContents function that returns an error message if the code being sent is too large, that is no longer reachable with existing tests and we need to create new ones so that we can be prepared for this event if it were to happen in production code.

Acceptance Criteria
There is a test that covers the code in opCompressor.ts

Execution Plan
Create a new test inside opCompressor.ts that has the ability to reach said code.

Reviewer Guidance

Let me know if there's a better way to go about doing this.

@github-actions github-actions bot added area: runtime Runtime related issues base: main PRs targeted against main branch labels Mar 10, 2025
Copy link
Member

@markfields markfields left a comment

Choose a reason for hiding this comment

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

Let's clarify the conditions we expect this error and how to test them, I think you're testing an impossible case right now, if I'm not mistaken.

@@ -82,7 +82,7 @@ export class OpCompressor {
/**
* Combine the batch's content strings into a single JSON string (a serialized array)
*/
private serializeBatchContents(batch: IBatch): string {
public serializeBatchContents(batch: IBatch): string {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth adding a comment (and updating the type) that this will just have one element. So this will only add the two square bracket characters.

It's a tiny window where this step could be the one that yields a string that's too long.

Copy link
Member

Choose a reason for hiding this comment

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

So the test case you're adding isn't possible in reality because the compressor would bail if there's more than one message.

assert.throws(
() => {
// eslint-disable-next-line @typescript-eslint/no-unsafe-call
compressor.serializeBatchContents(batch);
Copy link
Member

Choose a reason for hiding this comment

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

You can call this even if it's private, you just have to work around the regular type. I can show you another example of this on the phone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: runtime Runtime related issues base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants