-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Adds full report to CLI and extension. #206
Conversation
it('has no aggregators failing when shortName is called', () => { | ||
return walkTree.then(aggregators => { | ||
aggregators.forEach(aggregator => { | ||
assert.doesNotThrow(_ => aggregator.shortName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when would accessing a property ever cause a throw (unless you are testing that aggregator is always defined)? or am I missing something :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confirming that all aggregators define a shortName, as the report uses it for the menu and to generate the anchor, e.g. #addtohomescreen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah we should use .ok
https://nodejs.org/api/assert.html#assert_assert_ok_value_message
or doesNotEqual undefined then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the base class for Aggregator throws. All we want to check is that they've overridden it in some way. I guess we could .ok
as an inverse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
--mobile Emulates a Nexus 5X (default=true) | ||
--load-page Loads the page (default=true) | ||
--output How to output the page(default=pretty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll need better docs for this, but 👍 for now
Looking good. A few minor things, but the major things remaining are the filename for the outputted file and if we can just have a single |
Ack on the filename bug, and I could unify the report classes, but the paths aren't the same. |
All done, and added some extra gubbins (with tests) for the printer. PTAL. |
FYI changes now include:
This means we can generate any type of output and either push it to I'm not doing any path friendliness to make sure that the output directory exists. We can definitely do this, but this PR is already big enough. |
} | ||
|
||
getReportHTML() { | ||
return fs.readFileSync(path.join(__dirname, './templates/report.html'), 'utf8'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\o/
should really have held off on Printer refactor for a separate PR :P I really want to land this today, but is it too late there for additional changes? |
*/ | ||
set outputMode(mode) { | ||
if (Printer._outputModes.indexOf(mode) === -1) { | ||
console.warn(`Unknown output mode ${mode}; using pretty`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably want the npmlog
dance for these console.warn
lines?
Brendan is taking over this PR |
closing this in favor of #225 |
No description provided.