Merge pull request #34 from Financial-Times/fix-status-fails

Fix race condition between request interception and response
Financial-Times · Mar 16, 2018 · 62b9a2d · 62b9a2d
2 parents 3d2d16d + d2f3743
commit 62b9a2d
Showing 1 changed file with 60 additions and 38 deletions.
diff --git a/lib/smoke/puppeteer-page.js b/lib/smoke/puppeteer-page.js
@@ -54,7 +54,6 @@ class PuppeteerPage {
 		this.browser = browser;
 		this.page = await this.browser.newPage();
 
-
 		//clear cookies for each page request
 		const cookies = await this.page.cookies(this.url);
 		await Promise.all(cookies.map(cookie => this.page.deleteCookie(cookie)));
@@ -91,51 +90,51 @@ class PuppeteerPage {
 			this.consoleMessages.push(message);
 		});
 
-			// Intercept page requests, we need to do this in order
-			// to set the HTTP method or post data
-			await this.page.setRequestInterception(true);
+		// Intercept page requests, we need to do this in order
+		// to set the HTTP method or post data
+		await this.page.setRequestInterception(true);
 
-			// Intercept requests so we can set the HTTP method
-			// and post data. We only want to make changes to the
-			// first request that's handled, which is the request
-			// for the page we're testing
-			let interceptionHandled = false;
+		// Intercept requests so we can set the HTTP method
+		// and post data. We only want to make changes to the
+		// first request that's handled, which is the request
+		// for the page we're testing
+		let interceptionHandled = false;
 
-			this.page.on('request', interceptedRequest => {
-				const overrides = {};
-				const url = interceptedRequest.url();
+		this.page.on('request', interceptedRequest => {
+			const overrides = {};
+			const url = interceptedRequest.url();
 
-				// For user requests, only apply the request headers to the initial URL request, and any whitelisted paths
-				if(this.user) {
-					if(this.requestHeaderPaths.find(path => new RegExp(path).test(url))) {
-						overrides.headers = Object.assign({}, this.requestHeaders);
-					}
-				} else {
-					//Otherwise apply headers for any URls with the same host
-					if(new URL(url).host === new URL(this.url).host) {
-						overrides.headers = Object.assign({}, this.requestHeaders);
-					}
+			// For user requests, only apply the request headers to the initial URL request, and any whitelisted paths
+			if(this.user) {
+				if(this.requestHeaderPaths.find(path => new RegExp(path).test(url))) {
+					overrides.headers = Object.assign({}, this.requestHeaders);
 				}
+			} else {
+				//Otherwise apply headers for any URls with the same host
+				if(new URL(url).host === new URL(this.url).host) {
+					overrides.headers = Object.assign({}, this.requestHeaders);
+				}
+			}
 
-				if (!interceptionHandled) {
-
-					overrides.method = this.method;
-					// Override the request POST data if present
-					if (this.postData) {
-						if(typeof this.postData === 'object') {
-							this.postData = JSON.stringify(this.postData);
-						}
-						overrides.postData = this.postData;
+			if (!interceptionHandled) {
 
-						if(!overrides.headers['Content-Type']) {
-							overrides.headers['Content-Type'] = 'application/json';
-						}
+				overrides.method = this.method;
+				// Override the request POST data if present
+				if (this.postData) {
+					if(typeof this.postData === 'object') {
+						this.postData = JSON.stringify(this.postData);
 					}
+					overrides.postData = this.postData;
 
-					interceptionHandled = true;
+					if(!overrides.headers['Content-Type']) {
+						overrides.headers['Content-Type'] = 'application/json';
+					}
 				}
-				interceptedRequest.continue(overrides);
-			});
+
+				interceptionHandled = true;
+			}
+			interceptedRequest.continue(overrides);
+		});
 
 
 		this.page.on('response', response => {
@@ -145,6 +144,8 @@ class PuppeteerPage {
 
 			if(url !== this.url) {
 				this.requests.push({ url, status });
+			} else if (!this.response) {
+				this.response = response;
 			}
 			// if this response is a redirect
 			if (REDIRECT_CODES.includes(status)) {
@@ -155,12 +156,33 @@ class PuppeteerPage {
 			}
 		});
 
-		const waitUntil = this.check.elements || this.check.networkRequests ? 'load' : 'domcontentloaded';
+		const waitUntil = (this.check.elements || this.check.networkRequests) ? 'load' : 'domcontentloaded';
 		this.response = await this.page.goto(this.url, { waitUntil });
+
+		// So horrible - because we have request interception turned on, I *think* this.response is sometimes null if it comes back before request.continue();
+		// If this happens, wait for the response interception to have stored it
+		if(!this.response) {
+			this.response = await this.waitForResponse();
+		}
+
 		if(this.check.cssCoverage) {
 			this.coverageReports = await this.page.coverage.stopCSSCoverage();
 		}
 
+
+
+
+	}
+
+	async waitForResponse () {
+		return new Promise((resolve) => {
+			const poll = setInterval(() => {
+				if(this.response && this.response.status) {
+					clearInterval(poll);
+					resolve(this.response);
+				}
+			}, 50);
+		});
 	}
 
 	get status () {