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

[FIXED] Correct input for "parse 0" test case #1334

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

YukiBobier
Copy link
Contributor

Resolve issue #1333

This change aligns the input with the name of the test case and ensures that the function is accurately tested with this input.

This change aligns the input with the name of the test case
and ensures that the function is accurately tested with this input.
@piotrpio piotrpio self-requested a review June 23, 2023 14:33
Copy link
Collaborator

@piotrpio piotrpio left a comment

Choose a reason for hiding this comment

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

Hey @YukiBobier , thanks for the PR, indeed this may have been confusing.

Just one thing - I think we should still have a test where the input is an empty string, just with better test case name ("parse empty string"). That would mean that we should have 2 test cases instead of 1 - one with "0" as input and other with "" as input.

@YukiBobier
Copy link
Contributor Author

Hey @YukiBobier , thanks for the PR, indeed this may have been confusing.

Just one thing - I think we should still have a test where the input is an empty string, just with better test case name ("parse empty string"). That would mean that we should have 2 test cases instead of 1 - one with "0" as input and other with "" as input.

Hi @piotrpio ,

Thanks for your feedback on this pull request.

I wanted to point out that there is already an existing test case that tests the function with an empty string as input. Here's the test case:

{
name: "empty string",
given: "",
expected: 0,
},

Given this, I've made the change to have the "parse 0" test case use the string "0" as input, as per the test case name.

If there's any misunderstanding on my part, please let me know.

@YukiBobier YukiBobier requested a review from piotrpio June 23, 2023 17:25
Copy link
Collaborator

@piotrpio piotrpio left a comment

Choose a reason for hiding this comment

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

@YukiBobier My bad, I missed that there was a test case for empty string already.

LGTM, thanks for cotribution!

@piotrpio piotrpio merged commit e9480bf into nats-io:main Jun 23, 2023
1 check passed
@YukiBobier YukiBobier deleted the fix-parse0-testcase branch June 24, 2023 02:59
@YukiBobier
Copy link
Contributor Author

@piotrpio No problem at all! I'm glad the changes look good to you. Thanks for your approval and for taking the time to review my pull request.

I'm really happy to contribute to this project and look forward to making further contributions.

@piotrpio piotrpio mentioned this pull request Jul 20, 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.

None yet

2 participants