jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/394476 )
Change subject: Chore: add fetch wrapper tests
......................................................................
Chore: add fetch wrapper tests
• Add Sinon types.
• Add tests for the Marvin fetch abstraction using Sinon. Expose the
underlying fetch implementation used as a seam that defaults to
unfetch. An alternative considered was using FakeServer but these were
not as performant when last tried.
• Rename fetch#fetch() to request() to avoid confusion with the new
fetch seam exposed. Since a request() symbol is already defined in
several modules, wildcard import the entire fetch module where needed
to avoid compilation error. The Marvin bundle size tests do not
complain.
Bug: T177681
Change-Id: If6702dfca8557127f889810145ae7adbe44d71e0
---
M package-lock.json
M package.json
A src/common/http/fetch.test.ts
M src/common/http/fetch.ts
M src/common/http/page-http-client.ts
M src/common/http/page-summary-http-client.ts
6 files changed, 73 insertions(+), 8 deletions(-)
Approvals:
Jhernandez: Looks good to me, approved
jenkins-bot: Verified
diff --git a/package-lock.json b/package-lock.json
index 46592ab..be6cd49 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -129,6 +129,12 @@
"@types/mime": "1.3.1"
}
},
+ "@types/sinon": {
+ "version": "4.0.0",
+ "resolved": "https://registry.npmjs.org/@types/sinon/-/sinon-4.0.0.tgz";,
+ "integrity":
"sha512-cuK4xM8Lg2wd8cxshcQa8RG4IK/xfyB6TNE6tNVvkrShR4xdrYgsV04q6Dp6v1Lp6biEFdzD8k8zg/ujQeiw+A==",
+ "dev": true
+ },
"@types/source-map": {
"version": "0.5.1",
"resolved":
"https://registry.npmjs.org/@types/source-map/-/source-map-0.5.1.tgz";,
diff --git a/package.json b/package.json
index 3810611..f0604d8 100644
--- a/package.json
+++ b/package.json
@@ -64,6 +64,7 @@
"@types/mocha": "2.2.44",
"@types/node": "8.0.47",
"@types/node-fetch": "1.6.7",
+ "@types/sinon": "4.0.0",
"@types/webpack-node-externals": "1.6.0",
"assets-webpack-plugin": "3.5.1",
"bundlesize": "0.15.3",
diff --git a/src/common/http/fetch.test.ts b/src/common/http/fetch.test.ts
new file mode 100644
index 0000000..6a8b439
--- /dev/null
+++ b/src/common/http/fetch.test.ts
@@ -0,0 +1,52 @@
+import * as assert from "assert";
+import { Headers, Response } from "node-fetch";
+import * as sinon from "sinon";
+import { ClientError, RedirectError, ServerError, request } from "./fetch";
+
+describe("fetch", () => {
+ describe(".request()", () => {
+ it("returns a response when successful", () => {
+ const expected = new Response(undefined, { status: 200 });
+ const fetch = sinon.stub().returns(Promise.resolve(expected));
+
+ return request("url", undefined, fetch).then(response => {
+ assert.deepEqual(response, expected);
+ });
+ });
+
+ it("throws a RedirectError when redirected", () => {
+ const response = new Response(undefined, {
+ status: 300,
+ // todo: add node-fetch Headers constructor definition.
+ headers: new (Headers as any)({ location: "destination" })
+ });
+ const fetch = sinon.stub().returns(Promise.resolve(response));
+
+ return request("url", undefined, fetch).catch((error: any) => {
+ assert.ok(error instanceof RedirectError);
+ assert.deepEqual(error.status, 300);
+ assert.deepEqual(error.url, "destination");
+ });
+ });
+
+ it("throws a ClientError when the client fails", () => {
+ const response = new Response(undefined, { status: 400 });
+ const fetch = sinon.stub().returns(Promise.resolve(response));
+
+ return request("url", undefined, fetch).catch((error: any) => {
+ assert.ok(error instanceof ClientError);
+ assert.deepEqual(error.status, 400);
+ });
+ });
+
+ it("throws a ServerError when the server fails", () => {
+ const response = new Response(undefined, { status: 500 });
+ const fetch = sinon.stub().returns(Promise.resolve(response));
+
+ return request("url", undefined, fetch).catch((error: any) => {
+ assert.ok(error instanceof ServerError);
+ assert.deepEqual(error.status, 500);
+ });
+ });
+ });
+});
diff --git a/src/common/http/fetch.ts b/src/common/http/fetch.ts
index 8fa568d..3bca889 100644
--- a/src/common/http/fetch.ts
+++ b/src/common/http/fetch.ts
@@ -30,14 +30,15 @@
* identically to fetch. Capturing redirects allows the server to respond with
* the appropriate status code and resolved URL.
*/
-export function fetch(
+export function request(
input: RequestInfo,
- init?: RequestInit
+ init?: RequestInit,
+ fetch = unfetch
): Promise<Response> {
// Setting the redirect mode to "error" doesn't appear to yield the status
// code so "manual" is used instead.
const redirect = server ? "manual" : undefined;
- return unfetch(input, { redirect, ...init }).then(response => {
+ return fetch(input, { redirect, ...init }).then(response => {
if (server && response.status >= 300 && response.status <= 399) {
const url = response.headers.get("location");
throw new RedirectError(response.status, url as string);
diff --git a/src/common/http/page-http-client.ts
b/src/common/http/page-http-client.ts
index 46d155e..0404240 100644
--- a/src/common/http/page-http-client.ts
+++ b/src/common/http/page-http-client.ts
@@ -9,7 +9,7 @@
import { RESTBase } from "../marshallers/restbase";
import HttpResponse from "./http-response";
import { RESTBaseRedirect } from "./restbase-redirect";
-import { fetch } from "./fetch";
+import * as fetch from "./fetch";
import reencodePathSegment from "./restbase-path-encoder";
//
https://en.wikipedia.org/api/rest_v1/#!/Mobile/get_page_mobile_sections_title_revision
@@ -50,8 +50,10 @@
endpoint: string,
unmarshal: (params: UnmarshalParams) => Type
): Promise<HttpResponse<Type>> {
- const headers = params.random ? RANDOM_HEADERS : PAGE_HEADERS;
- return fetch(url(params, endpoint), { headers })
+ return fetch
+ .request(url(params, endpoint), {
+ headers: params.random ? RANDOM_HEADERS : PAGE_HEADERS
+ })
.then(response =>
response
.json()
diff --git a/src/common/http/page-summary-http-client.ts
b/src/common/http/page-summary-http-client.ts
index 230d151..1070ea4 100644
--- a/src/common/http/page-summary-http-client.ts
+++ b/src/common/http/page-summary-http-client.ts
@@ -4,7 +4,7 @@
import { unmarshalPageSummary } from
"../marshallers/page-summary/page-summary-unmarshaller"; // eslint-disable-line
max-len
import HttpResponse from "./http-response";
import { RESTBaseRedirect } from "./restbase-redirect";
-import { fetch } from "./fetch";
+import * as fetch from "./fetch";
import reencodePathSegment from "./restbase-path-encoder";
// https://en.wikipedia.org/api/rest_v1/#!/Page_content/get_page_summary_title
@@ -34,7 +34,10 @@
// todo: this can actually return an empty response when redirect is false. Do
// we want to support it? Same question for the other redirect usages.
export const request = (params: Params): Promise<HttpResponse<PageSummary>> =>
- fetch(url(params), { headers: params.random ? RANDOM_HEADERS : PAGE_HEADERS
})
+ fetch
+ .request(url(params), {
+ headers: params.random ? RANDOM_HEADERS : PAGE_HEADERS
+ })
.then(response =>
response
.json()
--
To view, visit https://gerrit.wikimedia.org/r/394476
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If6702dfca8557127f889810145ae7adbe44d71e0
Gerrit-PatchSet: 10
Gerrit-Project: marvin
Gerrit-Branch: master
Gerrit-Owner: Niedzielski <[email protected]>
Gerrit-Reviewer: Jhernandez <[email protected]>
Gerrit-Reviewer: Niedzielski <[email protected]>
Gerrit-Reviewer: Sniedzielski <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
