Support selectors in config to validate page load or throw pageLoadError

[LarsKumbier]

E2E-Testing with lighthouse is not always as straight-forward as we'd like. Chrome might refuse to start temporarily, the debug-port might not be open, the tested service might have a hickup.

I'd like to introduce functionality for validating a page before performing a lighthouse analysis and would like to invalidate the complete report, if the validator fails (similar to the NO_FCP behavior).

Questions

1. Where would I best do this to keep within the lighthouse architecture? The NO_FCP-error is thrown quite early, before the gatherers are run - which was the place I'd thought it would fit best. Does it need to be implemented out-of-band by utilizing the webdriver to first navigate to the page first before attaching lighthouse to it? Where's the best place to implement it in the lighthouse architecture?

2. How to invalidate the complete report, similar to what NO_FCP does?

3. I would like to validate simply by providing two configuration options: Both consist of an array of html selectors (XPath or something similar) - one defines one or more selectors that need to be present to identify a good state (so lighthouse should perform a test, e.g. that at least three product results are shown on the page), the other defines one of more selectors that need to be present to identify a bad state (so lighthouse should abort with an error instead, e.g. an error message on the html page).

[patrickhulce]

thanks for filing @LarsKumbier! I think this sort of validation makes sense, but there won't be a way to do this check before the page has been loaded like you request. We'd want to be sure the page Lighthouse is looking at has these good/bad elements not just the page you loaded before invoking lighthouse has them, so it'll have to be after each pass has loaded.

Before we get into architecture, how does the rest of team feel about this living in core?

I'm imagining getPageLoadError just has support for some of these extra options on the pass pass.pageLoadValidators: {required: ['.product', 'footer'], invalid: ['.error-msg']}

I'm +1

[brendankenny]

(I've renamed to try to capture the issue, feel free to change again :)

Before we get into architecture, how does the rest of team feel about this living in core?

I'm imagining getPageLoadError just has support for some of these extra options on the pass pass.pageLoadValidators: {required: ['.product', 'footer'], invalid: ['.error-msg']}

I'm a tentative +1, but I'm also interested in others' opinions.

We have also found that it's easy to miss that a page didn't load correctly...in the past you might be getting correct-looking metrics but it turns out they were from testing e.g. the Chrome NET::ERR_CERT_DATE_INVALID interstitial page due to a misconfigured test server, and you may have only noticed if you happened to look at the screenshot filmstrip.

The checks in GatherRunner.getPageLoadError have slowly been added to try to catch these cases automatically (the Chrome errors you mention should all be caught or that's a bug to fix :), but yeah, site-specific bugs mostly won't be noticed by Lighthouse and it's easy for users to miss that something wasn't loading as expected (and that the metrics may therefore be wrong) in mass automated testing.

How to invalidate the complete report, similar to what NO_FCP does?

If getPageLoadError returns an error ("required selector '.product' not found" or whatever could be a new possible returned error), then

• all the gatherers and audits relying on that page-load pass are invalidated
• any further passes won't run
• if the error is marked as an lhrRuntimeError (like NO_FCP is) the report JSON will have a runtimeError entry and LH will exit with a non-zero exit code.

I think that @patrickhulce is right that you'd want this to be the per-pass since you might e.g. have different expected page content (or no content at all) when offline.

I definitely like the specifics of this proposal...it fits in well with the goals of the existing architecture, and element selectors are easy to check without much complexity but still have a lot of power to determine if a page load was correct or not.

My main hesitations:

• is this going to be used by more than one user? :) It's going to require a custom config, and even if we made it easier to use (if there were large demand we could someday add a CLI flag or DevTools/PSI API option), it isn't trivial configuration.
  • Since the proposed design seems to fit with the existing architecture so well, doesn't cause any unexpected dependencies, and would be easy to test, a low use count may still be ok since there's not much cost
• is it going to be hard for users to understand the Lighthouse loading model and when this will run? Decent docs might help, but any users of this will need to understand passes, when LH considers a page loaded, etc

[LarsKumbier]

We have also found that it's easy to miss that a page didn't load correctly...in the past you might be getting correct-looking metrics but it turns out they were from testing e.g. the Chrome NET::ERR_CERT_DATE_INVALID interstitial page due to a misconfigured test server, and you may have only noticed if you happened to look at the screenshot filmstrip.
The checks in GatherRunner.getPageLoadError have slowly been added to try to catch these cases automatically (the Chrome errors you mention should all be caught or that's a bug to fix :), but yeah, site-specific bugs mostly won't be noticed by Lighthouse and it's easy for users to miss that something wasn't loading as expected (and that the metrics may therefore be wrong) in mass automated testing.

This is exactly the situation we have been in some time now. We did miss changed requirements in some pages (e.g. necessary parameters in the URL), which resulted in testing 4xx or application error pages. While Lighthouse seem to have some good checks in place for general page problems, a lot of applications have specific errors that are not catchable without introspective in a general manner.

I think that @patrickhulce is right that you'd want this to be the per-pass since you might e.g. have different expected page content (or no content at all) when offline.

That sounds perfectly reasonable for me.

• is this going to be used by more than one user? :) It's going to require a custom config, and even if we made it easier to use (if there were large demand we could someday add a CLI flag or DevTools/PSI API option), it isn't trivial configuration.

We already have a huge list of products and projects that utilize lighthouse in automated testing environments. All of them have this problem and it's currently solved by ... well, telling the user to check the traces or filmstrips for themselves if the reports don't seem to fit expectations. This is also true if a page requires some actions before being able to check the performance (e.g. logging in first via the webdriver, as documented in the recipes).

It's fine for me to start with custom configuration when using lighthouse programmatically first and think about CLI-flags later. But I don't see a big problem there either, because the proposal would just consist of an array of strings, which are easy to define from the commandline - but again, it's a separate issue for me to be dealt with later and out of scope for this proposal.

• Since the proposed design seems to fit with the existing architecture so well, doesn't cause any unexpected dependencies, and would be easy to test, a low use count may still be ok since there's not much cost

I think the usage count will increase quite fast - this is a general problem in automated testing and currently does lead to questions on the validity of test results. We already have a hard time explaining the variations in the metrics to non-technical people, who often cast a shadow of doubt on the validity of the results and sudden jumps in the metrics due to performance checks of error pages do increase this problem.

• is it going to be hard for users to understand the Lighthouse loading model and when this will run? Decent docs might help, but any users of this will need to understand passes, when LH considers a page loaded, etc

The proposal doesn't change that; at best we draw more attention to the architecture, which might require a bit more detail anyway. Considering the architecture overview - especially the image:

Neither the image nor the page explain passes, which cases lead to lighthouse invalidating a pass, etc. Maybe we should add that information here as well and that there are default validators for passes, with some further explanation in a separate page (similar to gatherers, audits, traces, etc.).

The proposal's documentation should be added there as well.

Should I open up a new issue to enhance the documentation?

[LarsKumbier]

@brendankenny @patrickhulce any updates from your side? Can we move forward with having it in core? Do you need more info? What are the next steps?

[connorjclark]

Offering an alternative available today, while we continue hashing this out:

lighthouse <url> -GA=my-run
jq .MainDocumentContent my-run/artifacts.json | grep some-thing -q
# check exit code for success

Note, MainDocumentContent is just the initial HTML.

[patrickhulce]

We could proceed with this in core from my perspective. I agree with @LarsKumbier that the usage count would be substantial enough in automated testing situations (and pair nicely with lighthouse-ci) to address brendan's first concern and that documentation on LH loading is probably necessary to address his second concern whether this is implemented or not 😆

Anyone have strong objections to this living in core Lighthouse?

[LarsKumbier]

@patrickhulce Doesn't seem like there are objections. What are the next steps?

[patrickhulce]

Next steps would be a draft PR with an example API to encourage discussion on the ergonomics.

My proposal:

const config = {
  extends: 'lighthouse:default',
  passes: [{
    pageValidators: {requiredSelectors: ['.app'], invalidSelectors: ['.login']},
  }]
}

and add the logic to gather-runner here:

lighthouse/lighthouse-core/gather/gather-runner.js

Lines 195 to 203 in ee3a9df

	/**
	* Returns an error if the page load should be considered failed, e.g. from a
	* main document request failure, a security issue, etc.
	* @param {LH.Gatherer.PassContext} passContext
	* @param {LH.Gatherer.LoadData} loadData
	* @param {LH.LighthouseError|undefined} navigationError
	* @return {LH.LighthouseError|undefined}
	*/
	static getPageLoadError(passContext, loadData, navigationError) {

[LarsKumbier]

I like the config proposal - how about adding more examples with the CSS-Selector scheme?

.hero > .slide:nth-child(5) img - there's a hero with at least 5 slides with an image somewhere in the content.

Is there a template for the draft PR somewhere?

[patrickhulce]

Oh by draft PR I just meant one that doesn't spend too much time fixing tests/lint, etc but implements the minimum functionality and explains the usage (draft stage on GitHub to make sure folks don't submit the wrong type of feedback)

And sure feel free to add as many examples as you think are enough to get the point across :)

[connorjclark]

https://github.blog/2019-02-14-introducing-draft-pull-requests/ fyi

[patrickhulce]

We're interested in picking this up again as part of Fraggle Rock work

[LarsKumbier]

Feel free - I did not get the greenlight in the current project due to shifting priorities so far.