Page MenuHomePhabricator

[Cashtab] Run all unit tests and update snapshots on npm test
ClosedPublic

Authored by bytesofman on Dec 1 2023, 17:46.

Details

Summary

T3365

Snapshot mgmt in Cashtab has been an issue for some time. We initially had an automated approach where

  • npm test ran automatically on git commit
  • This would fail if snapshots required updating
  • Separately, a posttest script added @generated to all snapshot files

This was abandoned because

  • Requiring tests to pass before a commit was extremely annoying as a dev, since sometimes you just wanted to save your work
  • The precommit hook ran Cashtab tests when other repos changed
  • Another npm dependency was required
  • Most importantly, the issue wasn't really solved since sometimes npm test would miss failing snapshots

Back on the "no system" system for some time after this, we still have issues. I think that most of the issues are being caused by npm test occasionally missing some unit tests that have impacted snapshots, as npm test only runs unit tests from affected files using boilerplate logic. This logic has been observed to miss failing tests in the past. The CI runs all the tests, occasionally catching a missed snapshot, and throwing an error.

Remove the logic that runs a limited set of Cashtab tests locally so that locally running the tests will always run all the unit tests. Add logic so that snapshots are automatically updated when npm test is run locally.

Remove the @generated -- the ones left in Cashtab are from the legacy script, still in files that have not changed since this automation was removed. Snapshots should be reviewed to make sure their update was consistent with the code change, per jest best practices https://jestjs.io/docs/snapshot-testing#best-practices

So -- it is still possible for a dev to push a diff up that has failing snapshots. However, this also means the dev did not run npm test to check his work before pushing the diff up, which is the kind of thing CI is expected to catch.

Test Plan

npm test and observe all cashtab unit tests run
Change a component, e.g. add the text 'test test' on line 51 of Receive.js. Run npm test and observe snapshots automatically update.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cashtab-snapshots
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25800
Build 51180: Build Diffcashtab-tests
Build 51179: arc lint + arc unit

Event Timeline

improve comment and logic for watchAll

output from npm test after changing some text

image.png (610×974 px, 86 KB)

Fabien added a subscriber: Fabien.

It might be worth looking into running this as part of arc unit, so that you're sure that it's ran an up-to-date before arc diff succeeds

This revision is now accepted and ready to land.Dec 3 2023, 11:21