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

Report v2: check/x boolean audits, expand details failing audits, optimal val #1963

Merged
merged 10 commits into from
Apr 7, 2017

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented Apr 4, 2017

R: all

Before:

screen shot 2017-04-04 at 12 51 49 pm

After:

screen shot 2017-04-04 at 12 49 07 pm

  • Expands failing audits + expander arrows
  • Support for boolean audits with X/check
  • Renders optimalValue text on audit title. Final UI we'll need to work out.

@ebidel ebidel added the v2report label Apr 4, 2017
const isAudit = 'result' in auditOrCategory;
const tagName = isAudit ? 'details' : 'div';

// Score template. Audits get a details dropdown. Categories don't.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we wanted to, we could make the whole category collapsable. For now, I decide to keep it as v1.

}

element.appendChild(
this._renderScore(audit.result.score, title, audit.result.helpText, audit));
Copy link
Collaborator

Choose a reason for hiding this comment

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

why can't we stick to audit.score?

Copy link
Contributor Author

@ebidel ebidel Apr 4, 2017

Choose a reason for hiding this comment

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

That doesn't have boolean information. Booleans come back score: 0 re score: 100. The UI needs to know if it should render a number score or a boolean pass/fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah moved discussion to #1963 (comment) :)

@@ -31,6 +31,10 @@ const RATINGS = {
* @return {string}
*/
function calculateRating(value) {
if (typeof value === 'boolean') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

:( can we just keep that logic in the report generator?


// Score template. Audits get a details dropdown. Categories don't.
const tmpContainer = this._createElement('div');
tmpContainer.innerHTML = `<div class="lighthouse-score">
Copy link
Collaborator

Choose a reason for hiding this comment

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

😱

Copy link
Contributor Author

@ebidel ebidel Apr 4, 2017

Choose a reason for hiding this comment

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

Don't be scared. Completely safe since we control all inputs for the initial set. Also, after spending time working on the report, this has proven to be infinitely nicer to work with.

@@ -98,19 +102,42 @@ window.ReportRenderer = class ReportRenderer {
* @param {string} description
* @return {!Element}
*/
_renderScore(score, title, description) {
const element = this._createElement('div', 'lighthouse-score');
_renderScore(score, title, description, auditOrCategory) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the doctype lies! since we're shifting away from displaying high precision scores is this still necessary? what about a precision: binary|grade|numeric property or something for each audit and we can say goodbye to typeof

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Score could also be an object with value/precision props. Or maybetype for the latter. Examples:

category.score = {value: 100, precision: 'numeric'}
audit.score = {value: true, precision: 'binary'}
audit.score = {value: 'A', precision: 'grade'}

Copy link
Member

Choose a reason for hiding this comment

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

can someone sum up what was decided? My thoughts:

  • audits are going to score in some fashion, whether that's 0-100, bucketed into grades, or pass/fail. Even if we require all audits to report a score as a number, it's still going to be an integer in that interval, buckets of every 10ish numbers, or 0/100. It makes some sense then for the audit to declare how it scored (perhaps on audit.meta), even if the reporter changes how the score is presented.

  • If on the other hand (or in addition to the above), we allow specifying the precision type in the config (on one of the audits in categories.categoryName.audits) it seems like we'd need some way of converting the score reported by the audit into the new format. binary -> numeric is easy, but (as discussed), what's the true/false cutoff for numeric -> binary and where do you specify it?

Copy link
Collaborator

@patrickhulce patrickhulce Apr 4, 2017

Choose a reason for hiding this comment

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

Here's my take on what was discussed

Immediate actions:

  • add a scorePrecision or similarly named property to the performance audits meta object specifying a numeric precision
  • add default scorePrecision of binary to the result in the base Audit class
  • update ReportRenderer to use the audit.result.scorePrecision combined with audit.score to decide how to display (no more typeof score === 'boolean' outside of ReportGenerator)

Potential future course of action that need further discussion:

We remove score from the audit result object and instead have them return values that are compared to the declared target/point of diminishing returns/median values and a score is generated for them, boolean audits return whether they pass or fail as they do today and a score of 100/0 is generated for them. We explore the possibility of introducing letter grades and changing the score curves.

Copy link
Member

Choose a reason for hiding this comment

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

add a scorePrecision or similarly named property to the performance audits meta object specifying a numeric precision

should be noted that is basically just giving expectedValue a new name :)

It will be nice to always have a numeric score, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you mean renaming a property to align with it's meaning and changing the types of values it can hold then yes it's basically that :)

Copy link
Member

Choose a reason for hiding this comment

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

an enum is an enum :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@patrickhulce do you want to craft those changes in a PR? I am working on some other stuff with <template> and can rebase when that lands.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ebidel sure thing

Copy link
Collaborator

Choose a reason for hiding this comment

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

*/
_renderScore(score, title, description, auditOrCategory) {
const isAudit = 'result' in auditOrCategory;
const precision = isAudit ? 'binary' : 'category'; // TODO(ericbidelman): placeholder
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ebidel ebidel force-pushed the reportv2-more branch 2 times, most recently from 1b3504c to f4e5842 Compare April 5, 2017 18:11
@ebidel
Copy link
Contributor Author

ebidel commented Apr 5, 2017

Removed .innerHTML!

It's been replaced by templates.html which includes declared html partials (using <template>). It will also be the future home for ported handlebars formatters and other UI scaffolding. Templates get added to report-template.html using our injection technique. It's unclear how devtools will bring in templates.html.

This PR also introduces a DOM class to organize DOM-related code. It includes wrapper methods for setting .textContent and .classList so we never run into errors with querySelector'd returning null. Plan to add some renderer tests.

The typeof === 'boolean' stuff is also removed. I've preemptively hardcoded precision placeholders for audit and cateogry scores. For now, _renderAuditScore and _renderCategoryScore construct an object: score = {value: audit.score, precision: 'numeric'}. The expectation is that report-generator.js will setup this object when it creates results and we can remove those lines.

PTAL.

- rename good/poor -> pass/fail
- organize DOM related code
- placeholders for score precision
@ebidel
Copy link
Contributor Author

ebidel commented Apr 5, 2017

Now with scoringMode. PTAL

column.appendChild(
this._createElement('div', 'lighthouse-score__description')).textContent = description;
// Fill in the blanks.
const value = tmpl.querySelector('.lighthouse-score__value');
Copy link
Member

Choose a reason for hiding this comment

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

s/value/elem/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when for valueEl so disambiguate.

@ebidel
Copy link
Contributor Author

ebidel commented Apr 6, 2017

Added tests. I'm done.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

there's stuff i could bikeshed.. like the DOM library API and how the templates are added to the page. But on the whole I'm happy here. Let's keep it moving.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

some things


```sh
npm run start -- --output=json --output-path=lighthouse-core/test/results/sample_v2.json http://localhost:8080/dobetterweb/dbw_tester.html
```
Copy link
Member

Choose a reason for hiding this comment

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

should we add something about consider deleting any irrelevant changes from the diff, like exact timings, etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

## Iterating on the v2 report

This'll generate new reports from the same results json.
This will generate new reports from the same results json.
Copy link
Member

Choose a reason for hiding this comment

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

harsh

@@ -23,6 +23,7 @@ const REPORT_TEMPLATE = fs.readFileSync(path.join(__dirname, './report-template.
// TODO: Setup a gulp pipeline to concat and minify the renderer files?
const REPORT_JAVASCRIPT = fs.readFileSync(path.join(__dirname, './report-renderer.js'), 'utf8');
const REPORT_CSS = fs.readFileSync(path.join(__dirname, './report-styles.css'), 'utf8');
const REPORT_TEMPLATES = fs.readFileSync(path.join(__dirname, './templates.html'), 'utf8');
Copy link
Member

Choose a reason for hiding this comment

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

want to clean up our paths here while you're at this? :D fs.readFileSync(__dirname + '/templates.html'), 'utf8') works just fine

Copy link
Collaborator

Choose a reason for hiding this comment

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

does that work just fine on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

does that work just fine on windows?

Yeah, Windows understands / and the fs methods will take care of the rest. The only real worry (that I know of) is if you want to specify an absolute path from the root, which is different cross platform, but we're using __dirname so it's not an issue here

@@ -97,11 +98,14 @@ class ReportGeneratorV2 {
generateReportHtml(reportAsJson) {
const sanitizedJson = JSON.stringify(reportAsJson).replace(/</g, '\\u003c');
const sanitizedJavascript = REPORT_JAVASCRIPT.replace(/<\//g, '\\u003c/');
// Remove script in templates files just to be safe.
const sanitizedTemplates = REPORT_TEMPLATES.replace(/<script>([\s\S]*?)<\/script>/g, '');
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how much we should emphasize this; it doesn't strip event handlers, for instance. It's all our own markup, so I vote for not worrying about sanitization of the templates (we don't sanitize report-template.html so why should we do that with these? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you. I recall that el.innerHTML = '<div>just a string</div>'; was deemed unsafe and that's our own markup :) The argument from someonnnnne was that we may add things later and not realize the mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is different enough, though, because it's not markup assembled in (potentially) multiple places, it's all in one spot. So I think it's better on that front. I also don't like calling it sanitized unless it is actually sanitized :)

Copy link
Member

Choose a reason for hiding this comment

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

really think we should remove this, or call it "templatesWithScriptTagsRemoves" or something. Current version is easily worked around with an attribute on the script tag...I don't think this is an arms race we want to even start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/* eslint-env browser */
/* eslint indent: [2, 2, { "SwitchCase": 1, "outerIIFEBody": 0 }] */

(function() {
(function(self) {
Copy link
Member

Choose a reason for hiding this comment

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

IIFE is still pointless :P

});

it('does not fail when given null element argument', () => {
assert.doesNotThrow(() => {
Copy link
Member

Choose a reason for hiding this comment

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

can drop the surrounding assert.doesNotThrow and just execute it. If it throws the test will fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of like being explicit.

});

it('does not fail when given null element argument', () => {
assert.doesNotThrow(() => {
Copy link
Member

Choose a reason for hiding this comment

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

don't need doesNotThrow wrapper

it('fails when template cannot be found', () => {
const dom = new DOM(document);
assert.throws(() => {
const clone = dom.cloneTemplate('#unknown-selector'); // eslint-disable-line no-unused-vars
Copy link
Member

Choose a reason for hiding this comment

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

can you just not assign to a var to lose the eslint directive?

Copy link
Contributor Author

@ebidel ebidel Apr 6, 2017

Choose a reason for hiding this comment

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

so sloppy...but done!

});

describe('ReportRenderer V2', () => {
const window = self;
Copy link
Member

Choose a reason for hiding this comment

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

isn't the global window already set to self? Or...something? The globals are confusing here (and only used to access ReportRenderer)


it('should render an exception for invalid input', () => {
const renderer = new ReportRenderer(document);
const output = renderer.renderReport({});
Copy link
Member

Choose a reason for hiding this comment

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

Since we're not really validating anything right now this relies on implementation details. What about something explicit like

const output = renderer.renderReport({
  get reportCategories() { throw new Error() }
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@brendankenny
Copy link
Member

there's stuff i could bikeshed.. like the DOM library API and how the templates are added to the page. But on the whole I'm happy here. Let's keep it moving.

If it really is just bikeshedding stuff that's fine, but this is the kind of thing that gets written once and we're stuck with from then on unless it bothers you so much in the future you file a bug/change it.

Now's the time to bring it up, even if we get to it in a followup :)

* Dummy text for ensuring report robustness: </script> pre$`post %%LIGHTHOUSE_JSON%%
*/
window.ReportRenderer = class ReportRenderer {
class DOM {
Copy link
Member

Choose a reason for hiding this comment

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

feels like something I know ahead of time you're going to disagree with me on :) but I don't personally see the need for a separate DOM class (yet, at least), especially because the split with ReportRenderer doesn't seem completely clear. Anything wrong with these living there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I knew you were going to bring this up! I wasn't convinced adding a bunch of generic methods to ReportRenderer made sense either. They could live as standalone helper functions, but devtools may need to extend them later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@ebidel
Copy link
Contributor Author

ebidel commented Apr 6, 2017

PTAL. I think I got everything.


// Append audit details to to header section so the entire audit is within one <details>.
const header = element.querySelector('.lighthouse-score__header');
if (header) {
Copy link
Member

Choose a reason for hiding this comment

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

in what cases might there not be a header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was meant to protect against null querySelector results like methods. But removed since we don't care about that anymore.


// Exports
window.ReportRenderer = ReportRenderer;
window.DOM = DOM;
Copy link
Member

Choose a reason for hiding this comment

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

not needed anymore since you removed the IIFE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mocha needs it, but I've switched to proper module exports for Node env.

* @param {!CategoryJSON} category
* @return {!Element}
*/
_renderCategoryScore(category) {
Copy link
Member

Choose a reason for hiding this comment

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

should these also be _renderAuditResult and _renderCategoryResult?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually going to change "_populateResult" back to _populateScore. It's populating the score dom with values.

Copy link
Member

Choose a reason for hiding this comment

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

I'm actually going to change _populateResult back to _populateScore. It's populating the score dom with values

my issue was that if someone was going into the codebase to see where audits' titles and descriptions are set, they probably wouldn't think it would be in _populateScore. If the issue is the template's id, that can change too :P

This is all going to change a lot in the near future, so if you feel strongly we can defer

_renderAuditScore(audit) {
const tmpl = this._dom.cloneTemplate('#tmpl-lighthouse-audit-score');

const scoringMode = audit.result.scoringMode;
Copy link
Member

Choose a reason for hiding this comment

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

How we've gotten here has made sense, but it is weird that scoringMode is on audit.result but the score is on audit directly

}

/**
* @param {!DocumentFragment|Element} element DOM node to populate with values.
Copy link
Member

Choose a reason for hiding this comment

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

this is only ever called with a DocumentFragment, though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, but it support both

@@ -97,11 +98,14 @@ class ReportGeneratorV2 {
generateReportHtml(reportAsJson) {
const sanitizedJson = JSON.stringify(reportAsJson).replace(/</g, '\\u003c');
const sanitizedJavascript = REPORT_JAVASCRIPT.replace(/<\//g, '\\u003c/');
// Remove script in templates files just to be safe.
const sanitizedTemplates = REPORT_TEMPLATES.replace(/<script>([\s\S]*?)<\/script>/g, '');
Copy link
Member

Choose a reason for hiding this comment

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

really think we should remove this, or call it "templatesWithScriptTagsRemoves" or something. Current version is easily worked around with an attribute on the script tag...I don't think this is an arms race we want to even start.

AVERAGE: {label: 'average', minScore: 45},
POOR: {label: 'poor'}
FAIL: {label: 'fail'}
Copy link
Member

Choose a reason for hiding this comment

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

does PASS/FAIL work here? Seems weird to have a three state pass/fail system :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really a pass/fail system. Just for labeling/coloring right now. We'll probably need to expand in the future if we support things like grades. Seems fine for now.

Copy link
Member

Choose a reason for hiding this comment

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

Not really a pass/fail system. Just for labeling/coloring right now.

yeah, I was mostly just commenting on the names since PASS/FAIL is usually a binary state. Fine for now

} catch (e) {
return this._renderException(e);
static setText(element, text) {
if (element) {
Copy link
Member

Choose a reason for hiding this comment

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

still think we should remove. Earlier (and non-silent) failures are better failures.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

last things

AVERAGE: {label: 'average', minScore: 45},
POOR: {label: 'poor'}
FAIL: {label: 'fail'}
Copy link
Member

Choose a reason for hiding this comment

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

Not really a pass/fail system. Just for labeling/coloring right now.

yeah, I was mostly just commenting on the names since PASS/FAIL is usually a binary state. Fine for now

}

/**
* @param {!DocumentFragment|Element} element DOM node to populate with values.
Copy link
Member

Choose a reason for hiding this comment

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

{!DocumentFragment|!Element} then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* @param {!CategoryJSON} category
* @return {!Element}
*/
_renderCategoryScore(category) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm actually going to change _populateResult back to _populateScore. It's populating the score dom with values

my issue was that if someone was going into the codebase to see where audits' titles and descriptions are set, they probably wouldn't think it would be in _populateScore. If the issue is the template's id, that can change too :P

This is all going to change a lot in the near future, so if you feel strongly we can defer

element.appendChild(this._renderAuditScore(audit));

// Append audit details to to header section so the entire audit is within one <details>.
const header = element.querySelector('.lighthouse-score__header');
Copy link
Member

Choose a reason for hiding this comment

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

since this depends on markup introduced by _renderAuditScore, move header/details stuff in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

<div class="lighthouse-score__snippet">
<span class="lighthouse-score__title"><!-- fill me --></span>
</div>
<div class="lighthouse-score__description"><!-- fill me --></div>
Copy link
Member

Choose a reason for hiding this comment

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

worth putting in a comment(s) to show where audits (and details for the below template) will go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea, but right now it's not worth it. Things are going to move around as we iterate. Don't want comments to get out of whack.

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea, but right now it's not worth it. Things are going to move around as we iterate. Don't want comments to get out of whack.

you say that now, but we'll have to see if anyone remembers later on :)

const page = jsdom.jsdom(new ReportGeneratorV2().generateReportHtml({}));
const templates = jsdom.jsdom(TEMPLATES_FILE);

assert.deepEqual(page.querySelectorAll('template[id^="tmpl-"]'),
Copy link
Member

Choose a reason for hiding this comment

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

deepStrictEqual? Deep equality checking also gets weird with e.g. empty objects and properties on prototypes (it doesn't check them), so it may be worth doing an explicit comparison with the expected elements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ebidel
Copy link
Contributor Author

ebidel commented Apr 7, 2017

PTAL

const page = jsdom.jsdom(new ReportGeneratorV2().generateReportHtml({}));
const templates = jsdom.jsdom(TEMPLATES_FILE);

assert.deepStrictEqual(page.querySelectorAll('template[id^="tmpl-"]'),
Copy link
Member

Choose a reason for hiding this comment

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

just checked, assert.deepStrictEqual also doesn't work here (besides checking that there's the same number of templates) because Object.keys() on the two HTMLTemplateElements returns [], so any two HTMLTemplateElements are considered deep equal. You'll probably need to manually check the innerHTMLs are equal or something

Copy link
Member

Choose a reason for hiding this comment

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

could do something like

const page = jsdom.jsdom(new ReportGeneratorV2().generateReportHtml({}));
const templates = jsdom.jsdom(TEMPLATES_FILE);

const pageTemplates = page.querySelectorAll('template[id^="tmpl-"]');
const rawTemplates = templates.querySelectorAll('template[id^="tmpl-"]');
rawTemplates.forEach((expectedTemplate, index) => {
  assert.strictEqual(pageTemplates[index].innerHTML, expectedTemplate.innerHTML);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switched to using lengths.

Copy link
Member

Choose a reason for hiding this comment

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

switched to using lengths

lulz, well the assert.deepEqual was checking that, but I guess at least now it's explicit

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM 🏗☑️

@brendankenny brendankenny merged commit 0b9f960 into master Apr 7, 2017
@brendankenny brendankenny deleted the reportv2-more branch April 7, 2017 23:55
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