bootstrap icon indicating copy to clipboard operation
bootstrap copied to clipboard

SCSS testing of the utilities API

Open romaricpascal opened this issue 2 years ago • 2 comments

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 the scss/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 running npm 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?

If really we want Jasmine, the way .scss files are compiled to tests are similar to what @babel/register does so we could follow the same set up they describe for transpiling tests for React

Add tests for the generate-utility mixin

I 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 right rfs-... function, but these tests could do with some reworking maybe.

I also noticed a couple of oddities while testing:

  • when using a list value for properties, the local-vars are printed for each of them. Feels like only once at the top of the rule should be enough, I'd believe?
  • when using states with local-vars, the variables are only set for the class without the state. Wondering if that wouldn't cause issue for a class like text-danger-hover if there's no text-... set when not hovered for example, but didn't take the time to test it.
  • local-vars are also not set when using css-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 import

Takes a small utilities configuration to check that the responsive and print 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

romaricpascal avatar Mar 16 '22 17:03 romaricpascal

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 watch-css-test script as Jasmine doesn't seem to provide a --watch option. Besides that, the setup is pretty much the same to the one described in the original PR for Mocha.

The code actually calling sass-true has been extracted into its own module (runner.js) so it could be linted like the rest of the JavaScript of the project.

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 number-leading-zero). There may be need for a few stylelint-disable in the expected output of tests, but I think it's better than configuring sass-true to its compressed mode as Bootstrap explicitely sets the expanded. It'll require caution if running npm run css-lint-stylelint -- --fix though.

romaricpascal avatar Mar 18 '22 09:03 romaricpascal

Just a heads up that I'll be jumping back into this soon! <3

mdo avatar Apr 06 '22 17:04 mdo