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.

Sign up for GitHub

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

feat: make browsertest failures standout #4148

Closed
wants to merge 1 commit into from
Closed

feat: make browsertest failures standout #4148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions lib/cli/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ exports.handler = argv => {
const destdir = argv.path;
const srcdir = path.join(__dirname, '..', '..');
fs.mkdirSync(destdir, {recursive: true});
const css = fs.readFileSync(path.join(srcdir, 'mocha.css'));
const css = fs.readFileSync(path.join(srcdir, 'mocha.sass'));
const js = fs.readFileSync(path.join(srcdir, 'mocha.js'));
const tmpl = fs.readFileSync(
path.join(srcdir, 'lib', 'browser', 'template.html')
);
fs.writeFileSync(path.join(destdir, 'mocha.css'), css);
fs.writeFileSync(path.join(destdir, 'mocha.sass'), css);
fs.writeFileSync(path.join(destdir, 'mocha.js'), js);
fs.writeFileSync(path.join(destdir, 'tests.spec.js'), '');
fs.writeFileSync(path.join(destdir, 'index.html'), tmpl);
Expand Down
2 changes: 1 addition & 1 deletion lib/reporters/html.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ function HTML(runner, options) {
hideSuitesWithout('test fail');
}
});

Copy link
Member

Choose a reason for hiding this comment

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

[Style] Nit: unnecessary change?

root.appendChild(stat);
root.appendChild(report);

Expand Down Expand Up @@ -210,6 +209,7 @@ function HTML(runner, options) {
);
}

stat.className += ' fail';
self.addCodeToggle(el, test.body);
appendToStack(el);
updateStats();
Expand Down
12 changes: 11 additions & 1 deletion mocha.css → mocha.sass
View file
Open in desktop
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at the styles:

Pass Fail
Screenshot of passing test with the PR's green background on the stats header Screenshot of failing test with the PR's red background on the stats header

A few things that make me not a fan yet:

  • The new colors are hard to read on top of, to the point of not having accessible color contrast.
  • The colors also don't match the existing aesthetic of the page. The page right now has very minimalist colors and a subtle design. This new green and red is pretty glaring. We should go with something that doesn't clash so much.
  • It's a little inconsistent in how its laid out: more padding on the top vs. the bottom, and less horizontal padding than other UI elements.

I'm not sure what would be the "right" approach. A few things come to mind, but I haven't thought very deeply:

  • Using much thinner border lines and/or much less saturated colors
    • Even switching to an nprogress-style bar on the stats header or the page
  • Instead of a background or border, using bold & colored text

(I'm not convinced any of that is better, just ideating)

Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,18 @@ body {
right: 10px;
font-size: 12px;
margin: 0;
color: #888;
color: #1d1e1f;
z-index: 1;
background-color: #00e600;
border: solid #00e600;
border-radius: 10px;
}

#mocha-stats.fail {
@extend #mocha-stats;
Copy link
Member

Choose a reason for hiding this comment

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

[Style] I don't think this @extend is really needed. We can achieve the same result in raw CSS, such as with CSS variables or different class names.

Thinking in terms of semantic class names, I think it'd make sense to have a class like .fail, .pass, etc. that each define their own colors.

Answering #4148 (comment): no, CSS can overcome these issues. 🙂

border: solid #c00;
background-color: #c00;
border-radius: 10px;
}

#mocha-stats .progress {
Expand Down
76 changes: 52 additions & 24 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@
"rollup-plugin-node-globals": "^1.4.0",
"rollup-plugin-node-polyfills": "^0.2.1",
"rollup-plugin-visualizer": "^4.1.0",
"sass": "^1.39.2",
Copy link
Member

Choose a reason for hiding this comment

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

[Structure] sass is a pretty big dependency to use for exactly one @extend. I don't think it's justified here. Let's revert the change from .css to .sass, unless there's a strong reason for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed in #5218 ;)

"sinon": "^9.0.3",
"strip-ansi": "^6.0.0",
"svgo": "^1.3.2",
Expand All @@ -167,7 +168,7 @@
"assets/growl/*.png",
"lib/**/*.{js,html,json}",
"index.js",
"mocha.css",
"mocha.sass",
"mocha.js",
"mocha.js.map",
"mocha-es2018.js",
Expand Down
2 changes: 1 addition & 1 deletion test/integration/init.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ describe('init command', function() {
return done(err);
}
expect(result, 'to have succeeded');
expect(fs.existsSync(path.join(tmpdir, 'mocha.css')), 'to be true');
expect(fs.existsSync(path.join(tmpdir, 'mocha.sass')), 'to be true');
expect(fs.existsSync(path.join(tmpdir, 'mocha.js')), 'to be true');
expect(fs.existsSync(path.join(tmpdir, 'tests.spec.js')), 'to be true');
expect(fs.existsSync(path.join(tmpdir, 'index.html')), 'to be true');
Expand Down