Fix recording query strings, where value is an array

[s-taylor]

This PR fixes an issue when using arrays inside a query string.

Prior to this change when using the following query string via superagent we get...
.query({"foo":["bar","baz"]})
The result is...
.query({"foo":"baz"})

Essentially the last value overrides any prior values due to the way the query string object is being formed. The fix is to simply use the qs library's .parse function which makes sense since the library is using qs elsewhere.

The result then becomes...
.query({"foo":["bar","baz"]})
Which now matches the superagent query input.

Resolves #896

[coveralls]

Coverage decreased (-0.005%) to 92.681% when pulling aa6053c on s-taylor:master into 17d7c25 on node-nock:master.

[s-taylor]

BTW I think the coverage error doesn't make sense, it's probably because I deleted lines. Which has reduced the overall number of lines and therefore makes it look as though the coverage has dropped.

My change is tested and all of the added lines will be covered by the test.

[s-taylor]

Any updates on this? I've had to use my forked version as recording doesn't match for us without this.

[travi]

i'd really appreciate getting this merged as well since this is the behavior that is expected for my project as well

[paulmelnikow]

The bug being fixed is that duplicate keys in the query string e.g. ?a=1&a=2&a=3 are lost. Is that correct?

[s-taylor]

Correct, it only records one value. The fix records this as an array.

[paulmelnikow]

Makes sense. Though the fix makes sense to me, the test seems very indirect. Could it be written in a simpler way?

[eddiemoore]

Would love to see this in nock as well.

[gr2m]

I’ve rebased the PR on the latest master.

I agree with @paulmelnikow that the test seems indirect, but it very much follows the conventions of the other tests around it. About time to clean them up for good, but that’s out of scope. Let’s wait to see if CI is happy and if it is, let’s merge it in

[coveralls]

Coverage decreased (-0.005%) to 92.666% when pulling dfa7dde on s-taylor:master into b050c49 on node-nock:master.

[gr2m]

sorry for the delay folks, I’ll release it as a new version right away

[gr2m]

released in nock@9.0.22

[travi]

Thanks @gr2m!

[s-taylor]

Thanks @gr2m , I would have been happy to add more tests but been busy and wasn't sure what additional tests you were after. Thanks for getting this merged!

[travi]

i confirmed that this solves the problem that we had. thanks again @gr2m and @s-taylor

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!