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

core(config): remove config.artifacts; always use auditMode #4986

Merged
merged 4 commits into from
Apr 18, 2018

Conversation

brendankenny
Copy link
Member

part of #4333

remove support for artifacts loaded in the config and the kind of wonky way it loaded traces and devtoolsLogs from there. Now the official way to load artifacts and skip the gather step is with --audit-mode/auditMode in the config.

Required creating some artifacts fixtures to port over old tests that loaded with config.artifacts, but on the plus side it means more tests for --audit-mode.

@@ -115,7 +113,8 @@ class Runner {
const resultsById = {};
for (const audit of auditResults) resultsById[audit.name] = audit;

let reportCategories;
/** @type {Array<LH.Result.Category>} */
let reportCategories = [];
Copy link
Member Author

Choose a reason for hiding this comment

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

part of the discussed change to default to having an empty categories array, even if none in the config (we could maybe start defaulting to [] in the config, but we'll see)

@@ -0,0 +1,14 @@
[
Copy link
Member Author

Choose a reason for hiding this comment

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

copied over from fixtures/traces/trace-user-timings.json cause it was small and we really only need to test the loading part :)

@@ -60,7 +60,7 @@ module.exports = {
},
beginDevtoolsLog() {},
endDevtoolsLog() {
return require('../fixtures/perflog.json');
return require('../fixtures/artifacts/perflog/defaultPass.devtoolslog.json');
Copy link
Member Author

Choose a reason for hiding this comment

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

this was big, so just moved over to the artifacts fixtures and moved the two tests using it to point at the new path

@@ -148,25 +148,6 @@ describe('Runner', () => {
});
});

it('accepts existing artifacts', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

overlap with lots of other -GA and auditMode tests, so removed

@@ -87,18 +87,17 @@ describe('Module Tests', function() {
});
});

it.skip('should return formatted audit results when given no categories', function() {
it('should return formatted LHR when given no categories', function() {
Copy link
Member Author

Choose a reason for hiding this comment

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

this was skipped when -GA was added. Reenabled :P

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.

huzzah!!! 🎉

// and to change TracingStartedInBrowser events into TracingStartedInPage.
// This is done by searching for most occuring threads and basing new events
// off of those.
function cleanTrace(trace) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what a beautiful sight :D

Copy link
Member

Choose a reason for hiding this comment

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

aye. this removes support for traces captured by webdriver, but at this point, i'm just fine with that. :)

@@ -782,11 +666,6 @@ class Config {
return this._audits;
}

/** @type {Array<!Artifacts>} */
get artifacts() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉

@brendankenny brendankenny merged commit 84debe2 into master Apr 18, 2018
@brendankenny brendankenny deleted the auditMode branch April 18, 2018 16:45
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.

4 participants