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 test case for ParseArrayPipe that doesn't work as expected #11170

Merged
merged 2 commits into from
Mar 10, 2023

Conversation

thgh
Copy link
Contributor

@thgh thgh commented Feb 27, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe: added a test case to see if it would break or not

What is the current behavior?

ParseArrayPipe is parsing the input as number even though String is requested.

Issue Number: N/A

What is the new behavior?

Not implemented, but it should parse only string.

Does this PR introduce a breaking change?

  • Yes
  • No, maybe

Other information

@thgh
Copy link
Contributor Author

thgh commented Feb 27, 2023

On further inspection, looks like array-pipe cannot handle anything JSON-like.

item = JSON.parse(item);

Comment on lines 179 to 180
await target.transform('1,2.0,3', {} as ArgumentMetadata),
).to.deep.equal(['1', '2.0', '3']);
Copy link
Member

Choose a reason for hiding this comment

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

So we won't merge this because this test won't pass.

image

Could you please open an issue or a PR to address this bug instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I opened an issue to discuss this peculiar behaviour.

@depak379mandal
Copy link

@thgh I think @micalevisk is asking for solution with testing rather than just test case.

@thgh
Copy link
Contributor Author

thgh commented Mar 9, 2023

Alright, fix committed!

@kamilmysliwiec kamilmysliwiec merged commit 8b72c52 into nestjs:master Mar 10, 2023
@kamilmysliwiec
Copy link
Member

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants