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

smoketests proposal: one test file per url #11950

Closed
paulirish opened this issue Jan 13, 2021 · 3 comments
Closed

smoketests proposal: one test file per url #11950

paulirish opened this issue Jan 13, 2021 · 3 comments
Assignees

Comments

@paulirish
Copy link
Member

paulirish commented Jan 13, 2021

problem

right now we have the https://github.com/GoogleChrome/lighthouse/tree/master/lighthouse-cli/test/smokehouse/test-definitions and within each expectation file there are >=1 tests. (aka individualTests)

as of aug 2020, we have 39 URLs we load. and they're defined across 16 files. (the 16 are selectable as 'pwa', 'dbw' 'pwa2', etc etc).

we have this yarn smoke dbw method of filtering but then if there are 4 URLs within dbw and we only want to run one of them... it usually leads to commenting out 90% of a file. not great.

proposal

each "expectation" file has just one URL.

we can still use slugs like tti-tricky or seo-tap-targets as the suffix to the yarn smoke invocation. (and should typically match the relevant filenames). So that style of filtering is still good.

(@connorjclark points out we do have some shared code like this now, but that seems quite solvable. )

if implemented, i don't think we need #9676 (which adds --only-urls and --only-audits to smokehouse cli)


(originally proposed here #9676 (comment) )


edit: we'd need to help pubads not break: https://github.com/googleads/publisher-ads-lighthouse-plugin/tree/master/lighthouse-plugin-publisher-ads/test/smoke

@brendankenny
Copy link
Member

brendankenny commented Jan 13, 2021

we have this yarn smoke dbw method of filtering but then if there are 4 URLs within dbw and we only want to run one of them... it usually leads to commenting out 90% of a file. not great.

proposal

each "expectation" file has just one URL.

we can still use slugs like tti-tricky or seo-tap-targets as the suffix to the yarn smoke invocation. (and should typically match the relevant filenames). So that style of filtering is still good.

I mentioned this in #9676 (review) that I think this approach makes the most sense given the goals here.

My main issue is that we haven't proven ourselves to be great namers (witness yarn smoke pwa pwa2 pwa3 :P), so it's not clear to me that this will be super usable (but then again, neither is yarn smoke --only-urls http://localhost:10200/js-redirect.html?delay=2000&jsDelay=5000&jsRedirect=%2Fonline-only.html%3Fdelay%3D1000%26redirect%3D%2Fredirects-final.html%253FpushState vs yarn smoke --only-urls http://localhost:10200/js-redirect.html?delay=2000&jsDelay=5000&jsRedirect=%2Fonline-only.html%3Fdelay%3D1000%26redirect%3D%2Fredirects-final.html :)

I'll also add to the goal that I really like being able to do yarn smoke seo or yarn smoke perf and get a group of tests that match the area I'm currently changing, and I don't have to know I want yarn smoke redirects-count-3 and yarn smoke redirects-count-2-js-redirect-after-a-delay to get both js and non-js redirect smoke tests. Dividing up the smoke tests between github actions is also going to get slightly more annoying if they can't be group addressable in some way.

Maybe fuzzy id checking? Even test.id.startsWith(requestedId) would cover a lot of this case, and we could just try to categorize tests as we do now...yarn smoke pwa would run pwa-svgomg, pwa-caltrainschedule, pwa-airhorner, etc. Or we could go full glob or whatever.

@brendankenny
Copy link
Member

brendankenny commented Jan 13, 2021

we also opened up smokehouse for others to use (like plugins), and at least publisher-ads uses it, so we'll have to be mindful of who we're breaking/help them update as it's sort of part of our public API now.

In theory we could update without breaking (still have expectations be an array, but only have a single item in any of the core ones), but ideally we can simplify the type if we aren't going to have multiple tests in each file.

@brendankenny
Copy link
Member

this was fixed with #12818 and #12819

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

No branches or pull requests

4 participants