Add integration test for staging env w/ Puppeteer #843

LeoSL · 2019-10-05T17:42:26Z

Pull request checklist

Make sure you:

Signed the Contributor License Agreement
Followed the commit message guidelines

Short description of the change(s)

Fix #810

Installs Puppeteer as a dev dependency in package.json.
Requests a collection of webhint.io staging URLs and saves the response in a
Map() object.
Iterates over the Map and checks if is there any status different from 200 or
another unknown error.
If there is an error, throws it to interrupt the test execution

🕹 How to test

Run the script node helpers/integration-tests.js ✅
If you want to see the script finding an error you can add a bad URL to the URL collection.
Example: http://pudim.com.br/xunda.jpg

molant · 2019-10-05T23:00:19Z

helpers/integration-tests.js

+                            .finally(() => {
+                                if (index === (URLsToVerify.length - 1)) {
+                                    resolve(responses);
+                                    browser.close();


browser.close() returns a Promise.
I'm thinking of 2 possibilities, if you have a better one please go for it:

Add the condition after responses.set(). Although we might end up with a bit more nesting 🤔

Use async/await inside a for..of and test the pages one by one. This might make the code a bit easier even if it means the test will take a few more seconds (which is fine TBH).

What do you think?

I don't think we need to keep the responses in a Map.
As @molant says, you should use async/await inside a for..of and after each scan, print the status (that way we can see if it is failing more than one url) and after all the urls are scanned, throw and error if there was any exception (or maybe it is enough if we return an exitCode !== 0)

sarvaje

Also, IMO, we should check that running a scan is working too.

sarvaje · 2019-10-07T16:55:20Z

helpers/integration-tests.js

+                            .finally(() => {
+                                if (index === (URLsToVerify.length - 1)) {
+                                    resolve(responses);
+                                    browser.close();


I don't think we need to keep the responses in a Map.
As @molant says, you should use async/await inside a for..of and after each scan, print the status (that way we can see if it is failing more than one url) and after all the urls are scanned, throw and error if there was any exception (or maybe it is enough if we return an exitCode !== 0)

LeoSL · 2019-10-08T14:41:14Z

Also, IMO, we should check that running a scan is working too.

@sarvaje Can you please clarify what you mean by "check that running a scan is working"?

sarvaje · 2019-10-08T16:13:12Z

@sarvaje Can you please clarify what you mean by "check that running a scan is working"?

Sorry hehehe. I mean going to the /scanner and add some url e.g. https://example.com click on run scan and see if you get a status code 200.

Some times, everything looks good, but then when you scan an url, something is broken.

molant · 2019-10-08T16:34:39Z

There are a couple methods in puppeteer that make this relatively easy:

page.type: it accepts a css selector and the text to type on the input
page.click which accepts a css selector and will "click" on it

Because we are going to navigate to a page we will probably have to use page.waitForNavigation to make sure that we navigate to a results page and get a 200 status code. The docs page has a good example on how to use waitForNavigation in combination with click and such but I think it will be something similar to:

await page.goto('/scanner/');
await page.type('selector for input', 'url to analyze');
const [result] = await Promise.all([
  page.waitForNavigation(),
  page.click('selector for button')
]);

// check result here

LeoSL · 2019-10-08T23:46:46Z

@molant I've rebased from upstream master and did the following:

Refactored the code to use async/await syntax sugar instead of Promise chains
Isolated the scanner test

Let me know if the changes sound good to you.
Thanks!

molant

Looking good @LeoSL !
I just have one comment that I hope you can check for me. Otherwise this is almost done!

molant · 2019-10-09T20:00:28Z

helpers/integration-tests_deprecated.js

@@ -0,0 +1,53 @@
+const puppeteer = require('puppeteer');


We no longer need this file 🙂

Oh, I forgot that one! Thanks!

It looks like this file is still in the PR

molant · 2019-10-09T20:00:30Z

helpers/integration-tests/staticUrls.js

+    return new Promise((resolve) => {
+        let errorFound = false;
+
+        staticURLsToVerify.forEach(async (url, index) => {


I believe .forEach doesn't await so all the items in the array are called at the same time and thus starting multiple different instances of the browser. When testing this how many separate chrome windows did you see? Was it just one being opened and closed or several?

If you saw multiple instances then we should probably change this to something like this:

const runStaticTests = async () => { let errorFound = false; for (const url of staticURLsToVerify) { const resultStatus = await runPuppeteer(url); if (resultStatus !== 200) { errorFound = true; } } return errorFound; }

@molant I'm running the Chrome headless version so I don't observe the windows popping up - thus I can't answer your question about the instances.

When running on the CLI they appear pretty much sequential on all my tests. I'm assuming that if they're being fired sequentially the printing (console.log) order would change on an ad hoc basis.

If you're strong about the for..of I can check it out later but I believe it would require a lot of other logic changes because I wouldn't have access to the index.

As I suspected forEach doesn't await , I've been bitten by this in the past 😅

With the code I posted earlier you don't need access to the index. Because the urls are being analyzed sequentially, as soon as the for..of ends you can return errorFound. We also add the keyword async so no need to create a Promise object.

That's great, thanks @molant
I will do the fixes later today! 👍

@molant Done!

- Installs Puppeteer as a dev dependency on package.json. - Requests a collection of webhint.io staging URLs and saves the response in a Map() object. - Iterates over the Map and checks if is there any status different from 200 or another unknown error. - If there is any error, throws it to interrupt the test execution

- Also, run both static and scanner tests sequentially to a better console logging experience

LeoSL · 2019-10-11T03:38:09Z

Sorry, @molant forgot again to remove. Now I believe everything looks fine!

molant

Looks great @LeoSL. Thanks a lot!

@sarvaje @antross can I get a second 👍 to merge this?

sarvaje

Just one NIT, but nothing blocking.

helpers/integration-tests/scanner.js

antross · 2019-10-14T20:20:33Z

@molant Looks like we two approvals now. Is there anything else you wanted or is this good to merge?

molant · 2019-10-14T22:10:49Z

Good to merge. Haven't had the time to merge today. Can you please take care of it?

…

________________________________ From: Tony Ross <notifications@github.com> Sent: Monday, October 14, 2019 13:20 To: webhintio/webhint.io Cc: Antón Molleda; Mention Subject: Re: [webhintio/webhint.io] Add integration test for staging env w/ Puppeteer (#843) @molant<https://github.com/molant> Looks like we two approvals now. Is there anything else you wanted or is this good to merge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#843>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAEUDAVEGYOAST5C2AG376DQOTIBFANCNFSM4I5YVAXQ>.

LeoSL force-pushed the add-staging-integration-test branch from f4febc7 to 6c457a7 Compare October 5, 2019 17:43

molant reviewed Oct 5, 2019

View reviewed changes

sarvaje suggested changes Oct 7, 2019

View reviewed changes

molant mentioned this pull request Oct 8, 2019

Add integration tests into the Azure Pipeline #842

Closed

LeoSL force-pushed the add-staging-integration-test branch from 6c457a7 to 2e49c25 Compare October 8, 2019 23:45

molant reviewed Oct 9, 2019

View reviewed changes

LeoSL added 4 commits October 10, 2019 16:19

Convert Promises chain to async/await syntax

2ededd4

Execute a deeper and isolated scanner test

6d54958

Simplify static test run

441db95

- Also, run both static and scanner tests sequentially to a better console logging experience

LeoSL force-pushed the add-staging-integration-test branch from 2e49c25 to 441db95 Compare October 10, 2019 20:40

Delete integration-tests_deprecated.js

b3cc36d

molant approved these changes Oct 11, 2019

View reviewed changes

sarvaje approved these changes Oct 11, 2019

View reviewed changes

helpers/integration-tests/scanner.js Show resolved Hide resolved

antross merged commit 0ae8573 into webhintio:master Oct 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration test for staging env w/ Puppeteer #843

Add integration test for staging env w/ Puppeteer #843

LeoSL commented Oct 5, 2019

molant Oct 5, 2019

sarvaje Oct 7, 2019

sarvaje left a comment

sarvaje Oct 7, 2019

LeoSL commented Oct 8, 2019

sarvaje commented Oct 8, 2019 •

edited

Loading

molant commented Oct 8, 2019

LeoSL commented Oct 8, 2019

molant left a comment

molant Oct 9, 2019

LeoSL Oct 9, 2019

molant Oct 11, 2019

molant Oct 9, 2019

LeoSL Oct 9, 2019 •

edited

Loading

molant Oct 9, 2019

LeoSL Oct 9, 2019

LeoSL Oct 10, 2019

LeoSL commented Oct 11, 2019

molant left a comment

sarvaje left a comment

antross commented Oct 14, 2019

molant commented Oct 14, 2019 via email

Add integration test for staging env w/ Puppeteer #843

Add integration test for staging env w/ Puppeteer #843

Conversation

LeoSL commented Oct 5, 2019

Pull request checklist

Short description of the change(s)

Fix #810

🕹 How to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarvaje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeoSL commented Oct 8, 2019

sarvaje commented Oct 8, 2019 • edited Loading

molant commented Oct 8, 2019

LeoSL commented Oct 8, 2019

molant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeoSL Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeoSL commented Oct 11, 2019

molant left a comment

Choose a reason for hiding this comment

sarvaje left a comment

Choose a reason for hiding this comment

antross commented Oct 14, 2019

molant commented Oct 14, 2019 via email

sarvaje commented Oct 8, 2019 •

edited

Loading

LeoSL Oct 9, 2019 •

edited

Loading