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

tests(viewer): add pptr test for viewer #5025

Merged
merged 5 commits into from
Apr 26, 2018
Merged

tests(viewer): add pptr test for viewer #5025

merged 5 commits into from
Apr 26, 2018

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Apr 24, 2018

As we start major work again on the report, it'd be good to have an easy way to verify we aren't breaking viewer. While viewer does already have compile and (a few) basic unit tests, this PR rips off @wardpeet's hard work from #4640 to make a pptr integration test for viewer as well.

Ideally we'd extract some sort of "use puppeteer to test a live report interface" test and have each individual directory file only in charge of setup and teardown, but I think(?) this is sufficient for now as we have more pressing matters.

assert.deepStrictEqual(pageErrors, []);
});

it('should contain all categories', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

below is all basically identical to the extension test

} else {
// Otherwise, disallow file requests outside of LH folder or from node_modules
const filePathDir = path.parse(absoluteFilePath).dir;
if (!filePathDir.startsWith(lhRootDirPath) || filePathDir.includes('node_modules')) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drive-by tightening :)

await viewerPage.goto(viewerUrl, {waitUntil: 'networkidle2'});
const fileInput = await viewerPage.$('#hidden-file-input');
await fileInput.uploadFile(sampleLhr);
await viewerPage.waitForSelector('.lh-container');
Copy link
Contributor

@ebidel ebidel Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to set a smaller timeout for this and goto? Default is 30s. If the link fails, we should know about it sooner. Speed up the tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, yes, misread that as 3s and thought it sounded great :)


const selectors = {
audits: '.lh-audit,.lh-timeline-metric,.lh-perf-hint',
titles: '.lh-score__title, .lh-perf-hint__title, .lh-timeline-metric__title',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaces here too: '.lh-audit, .lh-timeline-metric, .lh-perf-hint',

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

};

it('should load with no errors', async () => {
assert.deepStrictEqual(pageErrors, []);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol. just check pageErrors.length === 0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just check pageErrors.length === 0

I got the idea from the audit errors check below when I had errors at first. The error diff prints out the errors the page was seeing (instead of x !== 0), which is actually pretty nice when things go wrong and you don't have to do an extra debug step to print them

browser = await puppeteer.launch({
headless: false,
executablePath: process.env.CHROME_PATH,
args: [],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kill?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


// start puppeteer
browser = await puppeteer.launch({
headless: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need headful?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need headful?

...we're testing as a user? I don't know, I copied from #4640 :) Does it really matter in this instance?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yea. That's because extensions dont work in headless. You could use headless here but I'm not sure it really matters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

headless! lets do it.

used it over there because extensions yes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

headless! lets do it.

done

});
viewerPage = await browser.newPage();
viewerPage.on('pageerror', pageError => pageErrors.push(pageError));
await viewerPage.goto(viewerUrl, {waitUntil: 'networkidle2'});
await viewerPage.goto(viewerUrl, {waitUntil: 'networkidle2', timeout: 3000});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will throw if they timeout. Do you want to catch those as errors?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will throw if they timeout. Do you want to catch those as errors?

I think it's fine? It'll throw and fail in before, which will fail the tests.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

headless but w/e the rest is fine or w/e

const auditErrors = await viewerPage.$$eval('.lh-debug', getDebugStrings, selectors);
const errors = auditErrors.filter(item => item.debugString.includes('Audit error:'));
const unexpectedErrrors = errors.filter(item => {
return !item.debugString.includes('Required RobotsTxt gatherer did not run');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you confirm the savedartifacts are out of date?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, no RobotsTxt artifact in artifacts.json


// start puppeteer
browser = await puppeteer.launch({
headless: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

headless! lets do it.

used it over there because extensions yes.

executablePath: process.env.CHROME_PATH,
});
viewerPage = await browser.newPage();
viewerPage.on('pageerror', pageError => pageErrors.push(pageError));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@patrickhulce
Copy link
Collaborator

travis seems to be unhappy though

@brendankenny brendankenny force-pushed the vwrpptr branch 3 times, most recently from 48085fb to 3154866 Compare April 25, 2018 20:51
@brendankenny
Copy link
Member Author

travis unhappiness was due to report changes in #5009. extension test got updates for that but this obviously did not :)

@paulirish paulirish merged commit 4b8b51d into master Apr 26, 2018
@paulirish paulirish deleted the vwrpptr branch April 26, 2018 02:08
kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018
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.

5 participants