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: refactor StringCoder tests #942

Closed
wants to merge 5 commits into from
Closed

test: refactor StringCoder tests #942

wants to merge 5 commits into from

Conversation

danielbate
Copy link
Member

Related to #753

This PR will:

  • Introduce a seperate test file for the service under test
  • Seperate test cases for encode and decode
  • Remove snapshot tests to bring expectations into the same test file
  • Remove redundant tests to more clearly report on code coverage

@danielbate danielbate requested a review from a team April 28, 2023 12:04
@danielbate danielbate self-assigned this Apr 28, 2023
@danielbate
Copy link
Member Author

Screenshot 2023-05-02 at 17 12 45

Comment on lines +73 to +92
// TODO: StringCoder should throw for these conditions?

// it('should throw when encoding a string that is too big', () => {
// const coder = new StringCoder(0);
// const invalidInput = STRING_MAX_DECODED;

// expect(() => {
// coder.encode(invalidInput);
// }).toThrow();
// });

// it('should throw when encoding a string that is too small', () => {
// const coder = new StringCoder(1);
// const invalidInput = STRING_MIN_DECODED;

// expect(() => {
// coder.encode(invalidInput);
// }).toThrow();
// });
});
Copy link
Member Author

Choose a reason for hiding this comment

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

@FuelLabs/sdk-ts should we be throwing for these conditions? Other coders throw for this behaviour, wheras StringCoder will instead just slice the input by the length that is past to the construtor.

Copy link
Member

Choose a reason for hiding this comment

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

My instinct says yes — we should throw.

Not sure how the Rust SDK handles this.

cc @digorithm.

Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

Approving despite this. Curious to see what the outcome will be. 🤔

Copy link
Member

@Dhaiwat10 Dhaiwat10 left a comment

Choose a reason for hiding this comment

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

🪄

@danielbate danielbate closed this by deleting the head repository May 3, 2023
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.

3 participants