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

Mitigate array-of-statuses' impact on JSON bodies #728

Merged
merged 1 commit into from
Aug 11, 2021
Merged

Mitigate array-of-statuses' impact on JSON bodies #728

merged 1 commit into from
Aug 11, 2021

Conversation

swantzter
Copy link
Contributor

fixes a regression in #715 / v6.1.4 which introduces a breaking change where any array passed as the first argument to .expect() is treated as an array of status codes, rather than as a body which was the previous behaviour. This tries to minimise the impact of status code arrays.

Given, this will still break if the expected body is an array of numbers, but it will at least break less, and perhaps array bodies can be deprecated/removed in the next major release providing proper notice of the change?

@niftylettuce niftylettuce merged commit 3c46dae into ladjs:master Aug 11, 2021
@niftylettuce
Copy link
Collaborator

v6.1.5 released, can you further improve this by ensuring that besides being a number, they are one of the valid status codes as well in the 200-500 range?

@benjosantony
Copy link
Contributor

[].every(val => typeof val === 'number')) still return true, so incases where we are expecting the body to be empty array, this wrongly identifies the intention to be for testing status codes.

Changing the condition to if (Array.isArray(a) && a.length > 0 && a.every(val => typeof val === 'number')) { should fix that. (Basically add a check to be falsy when array length is zero as .every is truthy for empty array https://en.wikipedia.org/wiki/Vacuous_truth)

@benjosantony
Copy link
Contributor

I have raised a PR for that #735 @niftylettuce

@swantzter swantzter deleted the fix/array-of-statuses-body branch August 12, 2021 07:26
@niftylettuce
Copy link
Collaborator

v6.1.6 released

@swantzter
Copy link
Contributor Author

thanks, I'm not sure limiting to 200-599 is the correct route to me, sure it'd probably mitigate more cases, but http status codes can technically be any number from 0 to 999, for example that's how the fetch spec treat then, that way it feels like it could just lead to more confusion why [200, 204] works, but suddenly when you add your own vendored status code [200, 204, 609] it doesn't

gcf-merge-on-green bot pushed a commit to GoogleCloudPlatform/functions-framework-nodejs that referenced this pull request Oct 5, 2021
[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [supertest](https://togithub.com/visionmedia/supertest) | [`6.1.3` -> `6.1.6`](https://renovatebot.com/diffs/npm/supertest/6.1.3/6.1.6) | [![age](https://badges.renovateapi.com/packages/npm/supertest/6.1.6/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/supertest/6.1.6/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/supertest/6.1.6/compatibility-slim/6.1.3)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/supertest/6.1.6/confidence-slim/6.1.3)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>visionmedia/supertest</summary>

### [`v6.1.6`](https://togithub.com/visionmedia/supertest/releases/v6.1.6)

[Compare Source](https://togithub.com/visionmedia/supertest/compare/v6.1.5...v6.1.6)

-   Merge pull request [#&#8203;735](https://togithub.com/visionmedia/supertest/issues/735) from benjosantony/master  [`28116f9`](https://togithub.com/visionmedia/supertest/commit/28116f9)
-   Add on to the mitigation in [ladjs/supertest#728 to support empty array response body  [`ed0f68d`](https://togithub.com/visionmedia/supertest/commit/ed0f68d)

### [`v6.1.5`](https://togithub.com/visionmedia/supertest/releases/v6.1.5)

[Compare Source](https://togithub.com/visionmedia/supertest/compare/v6.1.4...v6.1.5)

-   Merge pull request [#&#8203;728](https://togithub.com/visionmedia/supertest/issues/728) from swantzter/fix/array-of-statuses-body  [`3c46dae`](https://togithub.com/visionmedia/supertest/commit/3c46dae)
-   Mitigate array-of-statuses' impact on JSON bodies  [`5a6999a`](https://togithub.com/visionmedia/supertest/commit/5a6999a)

### [`v6.1.4`](https://togithub.com/visionmedia/supertest/releases/v6.1.4)

[Compare Source](https://togithub.com/visionmedia/supertest/compare/v6.1.3...v6.1.4)

-   Merge pull request [#&#8203;715](https://togithub.com/visionmedia/supertest/issues/715) from juanvillegas/feature/allows-expect-to-take-an-array  [`7e4645c`](https://togithub.com/visionmedia/supertest/commit/7e4645c)
-   Allows expect to accept an array of statuses  [`ab252f3`](https://togithub.com/visionmedia/supertest/commit/ab252f3)

</details>

---

### Configuration

📅 **Schedule**: "every 3 months on the first day of the month" (UTC).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box.

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/GoogleCloudPlatform/functions-framework-nodejs).
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

3 participants