-
-
Notifications
You must be signed in to change notification settings - Fork 78.7k
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
Add SCSS testing of the utilities API #36029
Conversation
scss/mixins/_utilities.scss
Outdated
@@ -1,6 +1,6 @@ | |||
// Utility generator | |||
// Used to generate utilities & print utilities | |||
@mixin generate-utility($utility, $infix, $is-rfs-media-query: false) { | |||
@mixin generate-utility($utility, $infix: '', $is-rfs-media-query: false) { |
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.
This just made the writing of the tests easier as there was no need to carry a ''
infix at each call.
@@ -0,0 +1,391 @@ | |||
$prefix: bs-; | |||
$enable-important-utilities: false; |
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.
It's not the same default as in Bootstrap, but makes the writing of the tests much smoother as there's no !important
to repeat. It also made more sense to me tin terms of how tests are written to test what happens when the feature gets enabled.
@@ -0,0 +1,393 @@ | |||
// stylelint-disable scss/dollar-variable-default, declaration-no-important |
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 may move that to overrides for tests files, otherwise, it'll be a bit tedious to carry around each test.
package.json
Outdated
@@ -57,6 +57,7 @@ | |||
"css-prefix-main": "postcss --config build/postcss.config.js --replace \"dist/css/*.css\" \"!dist/css/*.rtl*.css\" \"!dist/css/*.min.css\"", | |||
"css-prefix-examples": "postcss --config build/postcss.config.js --replace \"site/content/**/*.css\"", | |||
"css-prefix-examples-rtl": "cross-env-shell NODE_ENV=RTL postcss --config build/postcss.config.js --dir \"site/content/docs/$npm_package_config_version_short/examples/\" --ext \".rtl.css\" --base \"site/content/docs/$npm_package_config_version_short/examples/\" \"site/content/docs/$npm_package_config_version_short/examples/{blog,carousel,dashboard,cheatsheet}/*.css\" \"!site/content/docs/$npm_package_config_version_short/examples/{blog,carousel,dashboard,cheatsheet}/*.rtl.css\"", | |||
"css-test": "mocha --config scss/tests/.mocharc.js", |
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.
Please see if we can use jasmine. A new dependency as mocha where they pin all their deps is pretty annoying.
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.
Sure, I'll rework that to use Jasmine. Hopefully there'll be some overlap between its dependencies and the one pulled by Karma 😊 I could try set it to the same version pulled by Karma, if looking for as much overlap as possible. But it feels like it's something that'll be easily missed on a future upgrade...
package.json
Outdated
@@ -175,5 +178,6 @@ | |||
"peerDependencies": { | |||
"@popperjs/core": "^2.11.4" | |||
} | |||
} | |||
}, | |||
"dependencies": {} |
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.
unrelated change
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.
Whoop, didn't see that, sorry 😞 I'll tidy it up!
scss/tests/sass-true/register.js
Outdated
|
||
require.extensions['.scss'] = function (module, filename) { | ||
return module._compile(` | ||
var sassTrue = require('sass-true'); |
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.
Can you also switch to const and match the current JS style?
@@ -125,6 +127,7 @@ | |||
"hammer-simulator": "0.0.1", | |||
"hugo-bin": "^0.82.2", | |||
"ip": "^1.1.5", | |||
"jasmine": "^4.0.2", |
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.
How about using the latest jasmine 3.x so that we don't use duplicate packages? We can update to 4.x when karma-jasmine is updated too.
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.
Damn, I'm very confused as to which packages get pulled now. I had originally found a 3.x (3.6 I think), then this morning I looked in the node-modules before adding Jasmine and jasmine-core
was 4.0.1. I'll double check on the main branch after clearing node-modules 😄
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.
Oh, so on main, there's two jasmine-core
pulled:
- one inside
karma-jasmine
, which is 3.99.1 (brought from it requiring^3.6.0
) - one at the root of node-modules, which is
4.0.1
(this one I'm not sure of the origin, but I'd guesskarma-jasmine-html-reporter
with>=3.8
)
I'll try and align if I can 😄
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.
You should stick to 3.x which is what we specify. The v4.x is a dependency of a dependency, not a direct one.
39bfb6f
to
f848c45
Compare
I think it's in a better shape now. Apologies for the too eager opening for review, and thanks for the comments 😄 The switch to Jasmine introduced the need to add a The code actually calling sass-true has been extracted into its own module ( One thing to be aware of is that sass-true compiles in expanded mode by default, which clashes with the linting at time (for rules like |
Just a heads up that I'll be jumping back into this soon! <3 |
40fae1f
to
45bf211
Compare
45bf211
to
76bc979
Compare
@romaricpascal So sorry for dropping the ball here. I'd like to get this merged in so we can write some additional tests—any chance you can bring it up to speed with |
@mdo No worries, figured you have a lot of balls to juggle. I should be able to get it back in shape at some point during the next two weeks |
Use mocha to handle the heavy lifting of finding tests and running them. Mocha is made to look directly for SCSS files which are compiled thanks to Node's require.extensions mechanism.
Cherry picked the commits for the SCSS testing mentioned in #28496 and added some testing of the utilities API (as far as I could understand it).
Set up testing for the utilities API
I stuck with Mocha as we'd have needed a new dependency anyway. Jasmine is only there by transitive dependency from Karma, which points to an old-ish 3.6 version. So I figured I'd save myself time and stick with it.
I moved the configuration to a
scss/tests/.mocharc.js
file, both for visibility and so the set up could be commented. Test placement is pretty free, anything ending in.test.scss
or.spec.scss
is considered a test. I've put all tests under thescss/tests
folder following the conventions on the JS side (rather than collocating tests into a__tests__
folder), so this could maybe be tightened a little.In terms of npm scripts, there's only one new one (instead of the extra described in the original discussion):
css-test
. As the configuration tells Mocha to also watch non-test.scss
files, it'll relaunch them automatically when runningnpm run css-test -- --watch
. When developing you can also use Mocha's--grep
option to limit which tests you're running.The tests have been added to the CSS GitHub worflow. Maybe I should move them before the actual CSS build as they'll likely run quicker than the build and there's no real point building if the CSS tests fail?
Add tests for the
generate-utility
mixinI really went with what I understood of the API and the source code, going through each of the properties/settings. I'm likely to have missed things, though.
And more importantly, I was very unclear as to how to work with the
rfs
option: I wasn't sure how/what to configure to enter each of the 3 states the code handles (outside media query, inside media query with distinct value from the utility value, inside media query with utility). In the end I just checked that we were calling the rightrfs-...
function, but these tests could do with some reworking maybe.I also noticed a couple of oddities while testing:
properties
, thelocal-vars
are printed for each of them. Feels like only once at the top of the rule should be enough, I'd believe?state
s withlocal-vars
, the variables are only set for the class without the state. Wondering if that wouldn't cause issue for a class liketext-danger-hover
if there's notext-...
set when not hovered for example, but didn't take the time to test it.local-vars
are also not set when usingcss-var
, which was a surprise too.Also thought of some extra developments for the utilities, but I'll open a separate issue to discuss if they're relevant (teasing 😝 ).
Add test for the
utilities/api
importTakes a small utilities configuration to check that the
responsive
andprint
options are working as intended. I saw there was a bit around RFS there too. I haven't looked into it so it'll need an extra test.Refs #28496