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

Basic reporting #53

Merged
merged 7 commits into from
Jul 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
255 changes: 247 additions & 8 deletions package-lock.json

Large diffs are not rendered by default.

7 changes: 7 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"repository": "https://github.com/BrightspaceUI/testing.git",
"scripts": {
"lint": "eslint . --ext .js",
"start": "web-dev-server --root-dir ./.vdiff --open ./report/",
Copy link
Contributor

Choose a reason for hiding this comment

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

When we wrap the runner, can we expose something like this so it doesn't need to be added to every package.json for the consumers? Or maybe we run this at the end of a test run 🤔?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking the same thing but forgot to mention it -- yes! We either do it automatically for visual-diff runs that have more than 1 failure (but not in CI), or add a --open-report flag or something (or both).

"test": "npm run lint && npm run test:server && npm run test:browser",
"test:browser": "web-test-runner --files \"./test/browser/**/*.test.js\" --node-resolve --playwright",
"test:server": "mocha ./test/server/**/*.test.js",
Expand All @@ -15,11 +16,17 @@
"author": "D2L Corporation",
"license": "Apache-2.0",
"devDependencies": {
"@rollup/plugin-node-resolve": "^15",
"@web/dev-server": "^0.2",
"@web/rollup-plugin-html": "^2",
"chai": "^4",
"deepmerge": "^4",
"eslint": "^8",
"eslint-config-brightspace": "^0.23",
"lit": "^2",
"mocha": "^10",
"page": "^1",
"rollup": "^3",
"sinon": "^15"
},
"exports": {
Expand Down
133 changes: 133 additions & 0 deletions src/server/report/app.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
import './test-result.js';
import { css, html, LitElement } from 'lit';
import data from './data.js';
import page from 'page';

class App extends LitElement {
static properties = {
_filterFile: { state: true },
_filterTest: { state: true },
_mode: { state: true },
_overlay: { state: true }
};
static styles = [css`
table {
border-collapse: collapse;
}
td, th {
border: 1px solid #cdd5dc;
padding: 10px;
}
thead th {
background-color: #f9fbff;
}
tbody th {
font-weight: normal;
text-align: left;
}
`];
constructor() {
super();
this._mode = 'sideBySide';
this._overlay = true;
}
connectedCallback() {
super.connectedCallback();
this._root = new URL(window.location.href).pathname;
page(this._root, (ctx) => {
const searchParams = new URLSearchParams(ctx.querystring);
if (searchParams.has('file')) {
this._filterFile = searchParams.get('file');
if (searchParams.has('test')) {
this._filterTest = searchParams.get('test');
} else {
this._filterTest = undefined;
}
} else {
this._filterFile = undefined;
this._filterTest = undefined;
}
});
page();
}
render() {
let view;
if (this._filterFile !== undefined && this._filterTest !== undefined) {
const fileData = data.files.find(f => f.name === this._filterFile);
if (!fileData) {
view = html`<p>File not found: <b>${this._filterFile}</b>.</p>`;
} else {
const testData = fileData.tests.find(t => t.name === this._filterTest);
if (!testData) {
view = html`<p>Test not found: <b>${this._filterTest}</b>.</p>`;
} else {
view = this._renderTest(fileData, testData);
}
}
} else {
view = data.files.map(f => this._renderFile(f));
}
return html`
<header>
<h1>Visual-diff Results</h1>
</header>
<main>${view}</main>
`;
}
_goHome() {
page(this._root);
}
_handleModeChange(e) {
this._mode = e.target.options[e.target.selectedIndex].value;
}
_handleOverlayChange(e) {
this._overlay = e.target.checked;
}
_renderFile(file) {
return html`
<h2>${file.name}</h2>
<table>
<thead>
<tr>
<th>Test</th>
${data.browsers.map(b => html`<th>${b.name}</th>`)}
</tr>
</thead>
<tbody>
${file.tests.map(t => this._renderTestResultRow(file, t))}
</tbody>
</table>
`;
}
_renderTest(file, test) {
return html`
<h2>${test.name} (${(test.results.length - test.numFailed)}/${test.results.length} passed)</h2>
<div>
<label>Mode:
<select @change="${this._handleModeChange}">
<option value="sideBySide" ?selected="${this._mode === 'sideBySide'}">Side-by-side</option>
<option value="oneUpOriginal" ?selected="${this._mode === 'oneUpOriginal'}">One-up (original)</option>
<option value="oneUpNew" ?selected="${this._mode === 'oneUpNew'}">One-up (new)</option>
Comment on lines +109 to +110
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I really understand this terminology, I assume this is what Percy uses?

</select>
</label>
<label><input type="checkbox" ?checked="${this._overlay}" @change="${this._handleOverlayChange}">Show overlay</label>
</div>
${test.results.map(r => html`<d2l-vdiff-report-test-result browser="${r.name}" mode="${this._mode}" file="${file.name}" ?show-overlay="${this._overlay}" test="${test.name}"></d2l-vdiff-report-test-result>`)}
<div style="margin-top: 20px;">
<button @click="${this._goHome}">Back</button>
</div>
`;
}
_renderTestResultRow(file, test) {
const results = test.results.map(r => {
return html`<td>${r.passed.toString()}</td>`;
});
return html`
<tr>
<th><a href="./?file=${encodeURIComponent(file.name)}&test=${encodeURIComponent(test.name)}">${test.name}</a></th>
${results}
</tr>
`;
}
}
customElements.define('d2l-vdiff-report-app', App);
21 changes: 21 additions & 0 deletions src/server/report/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Visual Diff Report</title>
<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<style>
@import url('https://fonts.googleapis.com/css2?family=Roboto:wght@400;700&display=swap');
body {
font-family: 'Roboto', sans-serif;
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha still using a google font but just not Lato? 😛

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha, didn't want to think about styling anything at this point. We can do Lato, although since we can't actually use any Daylight things from core we're not tied to it.

font-size: 16px;
}
</style>
</head>
<body>
<d2l-vdiff-report-app></d2l-vdiff-report-app>
<script type="module" src="app.js"></script>
</body>
</html>
110 changes: 110 additions & 0 deletions src/server/report/test-result.js
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 component renders out the results for a particular browser for a particular test. They can be stacked or iterated or whatever we want later:

Screenshot 2023-07-04 at 12 23 24 PM

Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
import { css, html, LitElement, nothing } from 'lit';
import data from './data.js';

class TestResult extends LitElement {
static properties = {
browser: { type: String },
mode: { type: String },
file: { type: String },
showOverlay: { attribute: 'show-overlay', type: Boolean },
test: { type: String },
};
static styles = [css`
.side-by-side {
flex-direction: row;
flex-wrap: nowrap;
display: flex;
gap: 10px;
}
.side-by-side > div {
flex: 0 1 50%;
}
img {
max-width: 100%;
}
.diff-container {
background: repeating-conic-gradient(#cccccc 0% 25%, #ffffff 0% 50%) 50% / 20px 20px;
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be changing, but one thing I noticed poking around was that the images are scaling to fit the width of the whole window, making it really hard to tell what the size was. This background just fills what's left, so something like this makes it look like the screenshots needed to be resized when they didn't:
image

I'm assuming we'll eventually size the images to the exact size they are?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah agreed, and I already have this fixed in the branch I'm working in to add filtering.

I think the behaviour we want is for them to scale to fit the width of the window, but grow no larger than the actual size of the images. That way if you have a small viewport (or a huge image), it'll zoom out to fit everything. I think that works well most of the time, but if it ends up being a problem in certain scenarios we could always have a "show actual size" button or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that sounds good, it's kinda hard to tell in this state with these contrived tests. I suspect we'll still be tweaking the reports as we go through and convert core, to see what pieces are actually useful and what's missing, like you did with all the helper commands.

background-position: 0 0;
border: 1px solid #cccccc;
display: inline-block;
line-height: 0;
}
.overlay-container {
position: relative;
}
.overlay {
background: hsla(0,0%,100%,.8);
position: absolute;
top: 0;
left: 0;
}
.no-changes {
border: 1px solid #cccccc;
padding: 20px;
}
`];
render() {

const { browserData, fileData, resultData, testData } = this._fetchData();
if (!browserData || !fileData || !resultData || !testData) return nothing;

return html`
<h3>${resultData.name} v${browserData.version} (${resultData.duration}ms)</h3>
${this._renderBody(resultData)}
`;

}
_fetchData() {

const fileData = data.files.find(f => f.name === this.file);
if (!fileData) return {};

const testData = fileData.tests.find(t => t.name === this.test);
if (!testData) return {};

const resultData = testData.results.find(r => r.name === this.browser);
if (!resultData) return {};

const browserData = data.browsers.find(b => b.name === this.browser);
if (!browserData) return {};

return { browserData, fileData, resultData, testData };

}
_renderBody(resultData) {

if (!resultData.passed && resultData.info === undefined) {
return html`
<p>An error occurred that prevented a visual-diff snapshot from being taken:</p>
<pre>${resultData.error}</pre>
`;
}

const noChanges = html`<div class="no-changes">No changes</div>`;
const goldenImage = html`<img src="../${resultData.info.golden.path}" loading="lazy" alt="">`;
const newImage = html`<img src="../${resultData.info.new.path}" loading="lazy" alt="">`;

const overlay = (this.showOverlay && !resultData.passed) ?
html`<div class="overlay"><img src="../${resultData.info.diff}" loading="lazy" alt=""></div>` : nothing;

if (this.mode === 'sideBySide') {
const g = html`<div class="diff-container">${goldenImage}</div>`;
return html`
<div class="side-by-side">
${ resultData.passed ? noChanges : g }
<div class="diff-container overlay-container">${newImage}${overlay}</div>
</div>`;
} else {
return html`
<div>
<div class="diff-container overlay-container">
${(this.mode === 'oneUpOriginal') ? goldenImage : newImage}
${overlay}
</div>
</div>`;
}

}
}

customElements.define('d2l-vdiff-report-test-result', TestResult);
15 changes: 15 additions & 0 deletions src/server/rollup.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { rollupPluginHTML as html } from '@web/rollup-plugin-html';
import { join } from 'path';
import { nodeResolve } from '@rollup/plugin-node-resolve';
import { PATHS } from './visual-diff-plugin.js';

export default {
input: join(PATHS.VDIFF_ROOT, './report/temp/index.html'),
output: {
dir: join(PATHS.VDIFF_ROOT, 'report')
},
plugins: [
html(),
nodeResolve()
],
};
Loading