Skip to content

Conversation

@connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Dec 14, 2020

not pretty. potentially better than alternative (?), which is to ditch the FPS audit and make it a property of the LHR... (lhr.fullPageScreenshot).

why: CDT uses onlyCategories, which means this audit is always skipped because it belongs to no category :(

@connorjclark connorjclark requested a review from a team as a code owner December 14, 2020 23:56
@connorjclark connorjclark requested review from adamraine and removed request for a team December 14, 2020 23:56
@google-cla google-cla bot added the cla: yes label Dec 14, 2020
@patrickhulce
Copy link
Collaborator

patrickhulce commented Dec 15, 2020

Just throwing this alternative out there, put it in all of them just hidden? :)

@brendankenny
Copy link
Contributor

potentially better than alternative (?), which is to ditch the FPS audit and make it a property of the LHR... (lhr.fullPageScreenshot).

The property could go wherever feels good. The reasoning is that the desired behavior is basically exactly the same as stackPacks. Gathered and audited regardless of which audits are run or filtered out, and the results aren't used as an audit in the report, they're used to provide extra information in a bunch of other audits.

Just throwing this alternative out there, put it in all of them just hidden? :)

We could revive @adrianaixba's hidden audit PR :)

@connorjclark
Copy link
Collaborator Author

We could revive @adrianaixba's hidden audit PR :)

I'd prefer this, but how much work does that entail? If we are pressed for time this PR seems fine to me, for now.

@patrickhulce
Copy link
Collaborator

I'd prefer this, but how much work does that entail? If we are pressed for time this PR seems fine to me, for now.

Probably not worth it, IMO, but seemed like you were searching for alternatives :)

I'd say we can just explore the lhr.fullPageScreenshot style for v8 and this works until then.

@benschwarz
Copy link
Contributor

We use onlyAudits and onlyCategories quite a bit. FPS is one of the audits that's regularly skipped by our test verification/scheduler as it caused PROTOCOL_TIMEOUT or crashed Chrome for certain sites.

Are you saying that it'd not be configurable to opt out of it running?

@patrickhulce
Copy link
Collaborator

patrickhulce commented Dec 15, 2020

@paulirish
Copy link
Member

here i wrote you a test :)

    it('should keep uncategorized audits even if onlyCategories is set', () => {
      assert.ok(origConfig.audits.includes('full-page-screenshot'));
      // f-p-s is non-categorized
      const matchCategories = Object.values(origConfig.categories).filter(cat =>
          cat.auditRefs.find(ref => ref.id === 'full-page-screenshot'));
      assert.equal(matchCategories.length, 0);


      const extended = {
        extends: 'lighthouse:default',
        settings: {
          onlyCategories: ['accessibility'],
        },
      };
      const config = new Config(extended);

      assert.ok(config.audits.find(a => a.path.includes('full-page-screenshot')));
    });

Copy link
Contributor

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

Overall LGTM

@paulirish
Copy link
Member

coolio. @brendankenny is gonna write up an issue exploring our options for where we can take this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants