Skip to content

Add stringifyQuery unit test#76

Merged
mahdiyazdani merged 3 commits intomainfrom
Add-stringifyQuery-unit-test
Jul 15, 2021
Merged

Add stringifyQuery unit test#76
mahdiyazdani merged 3 commits intomainfrom
Add-stringifyQuery-unit-test

Conversation

@AdrianFiroiu
Copy link
Copy Markdown
Member

This PR proposes a test suite for the stringifyQuery utility. Please let me know if I should extend it with any further test cases.

@AdrianFiroiu AdrianFiroiu added the enhancement New feature or request label Jul 7, 2021
@AdrianFiroiu AdrianFiroiu self-assigned this Jul 7, 2021
Comment thread src/stringify-query/test/index.js Outdated
it.each( [
[ 'lorem', '0=l&0=o&0=r&0=e&0=m' ],
[ { product: '1885' }, '0,1,2,3=1,8,8,5' ],
[ [ { product: '1885', action: 'hook_name' } ], 'product,action=1885,hook_name' ],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@AdrianFiroiu I think it’s a valid example, and you can remove it. What is your opinion on it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well my initial thought behind this case starts from the fact that this utility expects the input to be an array of single key/value pair objects but if the object we supply has multiple key/value pairs, then the output is IMO invalid.

At least to me, the output 'product,action=1885,hook_name' does not represent a valid query string and I can't really see a use case for it. Please let me know if I could be wrong.

This could also be classified as a bug and we should actually support multiple key/value pairs in a single object?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes @AdrianFiroiu , I agree, it should support multiple values and keys. @kuserich @mahdiyazdani What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree. I have created the issue #80

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @kuserich. So, @AdrianFiroiu would you please move this to the Valid arrays and replace the equal value with a value that we expected to have?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @kuserich. So, @AdrianFiroiu would you please move this to the Valid arrays and replace the equal value with a value that we expected to have?

I would be in favour of leaving it as is and moving / replacing it when we fix the issue.

Copy link
Copy Markdown
Contributor

@kuserich kuserich left a comment

Choose a reason for hiding this comment

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

Thank you, @AdrianFiroiu . I have removed the test cases that do not provide an Array input as they are invalid by definition (function expects arrays).
I'm in favour of keeping the test case with multiple fields and update the test case in the same PR that updates the function.

@kuserich kuserich requested a review from gooklani July 11, 2021 12:59
Comment thread src/stringify-query/test/index.js Outdated
it.each( [
[ 'lorem', '0=l&0=o&0=r&0=e&0=m' ],
[ { product: '1885' }, '0,1,2,3=1,8,8,5' ],
[ [ { product: '1885', action: 'hook_name' } ], 'product,action=1885,hook_name' ],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @kuserich. So, @AdrianFiroiu would you please move this to the Valid arrays and replace the equal value with a value that we expected to have?

@mahdiyazdani mahdiyazdani merged commit e6f4f96 into main Jul 15, 2021
@mahdiyazdani mahdiyazdani deleted the Add-stringifyQuery-unit-test branch July 15, 2021 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants