refactor(postcss-reduce-initial): refactor and add tests for acquire script

[sigveio]

Supersedes #1160

Original description:

This pull request aims to refactor the acquire script under postcss-reduce-initial

The script is responsible for generating data used by the plugin to determine whether or not there is something to gain from replacing a CSS property value with either the initial keyword or a shorter equivalent.

The refactor sets out to do the following:

• Separate concerns into different files where reasonable
• Replace comments with more readable code
• Refactor with testability in mind
• Replace dependencies with more up-to-date and lightweight alternatives
• Add test coverage

Closes #1159

Dev-dependency changes:

Replaced:

got -> node-fetch
html2plaintext -> html-to-text

Removed:

write-file (Replaced with node.js' writeFile)

Added:

nock (Adds convenient HTTP server mocking for tests)

Overall dependency impact:

By my not so careful calculations, this yields a net result of approx. 25 transient dependencies removed.
(Most of the credit for that goes to @ludofischer for pointing out that got is dep. heavy)

And notably removes a vulnerable version of css-what - from the seemingly unmaintained html2plaintext

Motivation / Context

My original motivation was to replace html2plaintext due to a vulnerability in one of its dependencies. After looking into it more closely and getting feedback from @ludofischer in #1159 it seemed sensible to add a test suite so that the data files can be generated with confidence in the future (perhaps also automated as part of the build). I also saw an opportunity to make some structural changes to help aid readability/maintainability.

Are you introducing a breaking change?

• Yes
• No (refactors auxiliary script)

Additional notes:

• Test coverage does not (as pointed out by @ludofischer in the issue) currently protect fully from the source file format changing. Although perhaps unlikely to happen, since the source is the MDN CSS Data repository, it could perhaps be prudent to add snapshot testing of data/fromInitial.json and data/toInitial.json to serve as a basic guard against unexpected changes. (I am leaving this out until I have feedback on whether or not this is desired or not)
• The unrelated coverage ignore pattern !packages/postcss-colormin/src/generate.js was removed from jest.config.js as the file no longer seems to exist.
• A transform pattern was added to jest.config.js to support importing ES modules with .mjs extension in tests.

[codecov-commenter]

Codecov Report

Merging #1169 (163fec1) into master (506a823) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1169      +/-   ##
==========================================
+ Coverage   96.51%   96.55%   +0.04%     
==========================================
  Files         114      116       +2     
  Lines        3584     3627      +43     
  Branches     1052     1065      +13     
==========================================
+ Hits         3459     3502      +43     
  Misses        117      117              
  Partials        8        8              

Impacted Files	Coverage Δ
...kages/postcss-reduce-initial/src/script/lib/io.mjs	100.00% <100.00%> (ø)
...tcss-reduce-initial/src/script/lib/mdnCssProps.mjs	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 506a823...163fec1. Read the comment docs.

[sigveio]

Once you are happy with this and it is merged I will follow up with a PR for regenerating the data files, as per the discussion. Including the proposed mechanism for cleanly overriding initial values based on a separate file instead of direct edits to the generated files :-)

[ludofischer]

Good job!

[ludofischer]

@sigveio it looks like you know your way around testing frameworks. Did you ever try https://stryker-mutator.io/docs/stryker-js/introduction/? I've found that mutation testing is more accurate than just coverage when finding untested functionality, but the tools run very slowly.

[ludofischer]

It just occurred to me that this dependency should probably go into the postcss-reduce-initial package, it's not a workspace dependency.

[sigveio]

It seemed like @alexander-akait chose to move these dependencies from the individual packages out into the workspace in /pull/804. I assumed that it was to make maintenance/keeping them up to date simpler by managing it all in one place? Perhaps I misunderstood though; I'm not that familiar with the lerna universe yet

[ludofischer]

Interesting. I think at the time the repository did not use workspaces, so devDependencies would end up duplicated in every sub-package. If you put package-specific dependencies the workspace, when you run a command in that single package it might fail because it cannot find the dependency. For example right now, you cannot re-build a single package at a time because yarn cannot find rimraf since it is declared in the workspace (for example yarn workspace stylehacks run build). I guess it makes little practical difference in this case, since these are test dependencies and you cannot run the tests from inside a single package anyway.

[sigveio]

I guess it makes little practical difference in this case, since these are test dependencies and you cannot run the tests from inside a single package anyway.

Indeed, and the tool/script itself is also generally only useful in the context of maintaining the repo. Off the top of my head, I can't really think of any likely use cases where it would be used by itself. As it's not actually packaged/distributed with postcss-reduce-initial. But that perhaps also raises the question if these kind of scripts would be better suited living in /util together with the build and test helpers rather than under the package. Perhaps also something to think about if we circle back to the question of organisation at some point. 🤔

Come to think of it....

it's not actually packaged/distributed with postcss-reduce-initial

... the ignore pattern in the build command for this package appears to be something I have overlooked to update.

Edit: Added a fix for that in #1170

[ludofischer]

This one too should go into the postcss-reduce-initial package.

[sigveio]

@sigveio it looks like you know your way around testing frameworks. Did you ever try https://stryker-mutator.io/docs/stryker-js/introduction/? I've found that mutation testing is more accurate than just coverage when finding untested functionality, but the tools run very slowly.

Thanks for the tip =) Sounds potentially interesting for extra quality control. Perhaps especially for analysing existing coverage. But I don't have any experience with that one.

You mention speed; and this is one big reason to why I love working with Jest. Compared to many other testing frameworks it's blazingly fast (!!), which makes the developer experience so much better when you work in parallel with the tests. This for me is important.

I suppose I do a form of mutation testing manually while I create them - mutating both the test and the code to verify continuously as part of the process. Letting the test improve/guide the code, and vice versa.

I tend to lean towards both testing things in isolation and integrated together to ensure thorough coverage. And also try to think about how I can make tests that are as little as possible tied up to implementation specifics while asserting something meaningful. Plus the tests role in helping to document the code.

Of course it's unavoidable some times, but I feel that many get a bit too caught up in the internal details and end up with very fragile tests. Which makes it harder to have confidence in them, and kind of defeats the purpose a bit.

And I notice that in the world of parsers and build tools... there is perhaps a tendency to forget a bit about the actual unit tests and lean heavily on a form of e2e-test with looking at input -> expected output from the tool. Which requires very very thorough and extensive cases to test every possible variation/possible scenario. This can be difficult to get water tight. Something that also gets increasingly complicated for new developers to verify as time passes and complexity grows.

I think there is a lot of potential for false confidence in the high coverage numbers they achieve - from the code being indirectly "exercised" and logged as covered.

For example; recently I have been looking at postcss-merge-longhand as a potential next target for refactoring, bug fixing, and test coverage. And the seemingly most buggy part of that - the borders plugin - has 181 tests in a file of more than 1200 lines. But not as much as one test against any specific part of the code.

So in reality there is a lot of untested functionality, and hard to determine what is actually covered or not.

I think this is an area it would be worth to have a focus on gradually improving while we work on the various packages. I think it would generally help strengthen the codebase, and make it much easier to update things and add new features with confidence. 😺

[ludofischer]

OK, I am going to merge this as it is, we'll look at monorepo organisation at another time.