diff --git a/README.md b/README.md index 2138f84..c52b218 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,8 @@ automatically and are enumerated [below](#task-reference). ### Upgrading to 9.0.0 from 8.x -Two breaking changes, both requiring a small edit in the widget repo. +Two breaking changes requiring a small edit in the widget repo, plus three changes to what passes and +fails that need no edit but do change results. **1. gulp is gone.** Delete your `gulpfile.js`, drop `gulp` from your devDependencies, and change every npm script from `gulp ` to `rhtml `. Task names, sequences and command line flags are all @@ -79,6 +80,47 @@ byte for byte identical to what the gulp pipeline produced, which is deliberate, css feeds the pages the visual regression suite screenshots. `less` is pinned to 3.13.1 (the version `gulp-less@4` resolved) to keep it that way. +#### Three changes to what passes and fails + +These need no edit in your repo, but they will change your results, so sequence a bump deliberately. + +**A mismatching snapshot now fails its own test.** Previously the comparison error was swallowed, so a +test whose images did not match reported PASS and the job only went red via jest's aggregate count. +Reading the per-test list therefore led straight to the wrong conclusion. Expect previously-green runs +to surface real per-test failures. + +**A snapshot with no baseline now fails.** `acceptNewSnapshots` defaults to `false`. It used to default +to `true`, which appended `--ci=0` to the jest command and made jest write the missing baseline and pass +— so a newly added test could look green forever while never being regression-tested. Pass +`--acceptNewSnapshots` to opt back in when bootstrapping a suite. + +**`clean` no longer deletes `man/`.** That directory holds tracked roxygen output which only `makeDocs` +can regenerate, and `makeDocs` swallows its own failure so a missing R install is not fatal — so +`rhtml build` used to silently delete tracked R documentation on every machine without R on PATH, +including CI. `makeDocs` also now calls `Rscript` rather than `r`, which is what makes it capable of +succeeding on Windows at all. + +The `--env` flag also no longer has a `local`/`travis` whitelist, so a CI environment can be named after +the system running it instead of being set indirectly through `widget.config.js`. + +#### `crypto` is stubbed in the bundle + +**If your widget calls anything on node's `crypto`, you must opt back in.** Add to +`build/config/widget.config.js`: + + esbuildOptions: { alias: { crypto: 'crypto-browserify' } } + +Without it the first call throws with a message pointing back here, rather than failing silently. +**rhtmlPictographs is the known case** — `CacheService.js` and `SvgDefinitionManager.js` both use +`crypto.createHash`. + +Why the default changed: `bignumber.js@2` (rhtmlCombinedScatter, rhtmlLabeledScatter) reaches for crypto +via `require('cry' + 'pto')` inside a `try/catch`, an idiom specifically intended to stop bundlers +resolving it — and browserify duly shipped none of it. esbuild constant-folds the concatenation, so it +resolves, and mapping it to `crypto-browserify` dragged 616 KiB across 180 files (`elliptic`, four copies +of `bn.js`, `asn1.js`, `diffie-hellman`) into rhtmlCombinedScatter's bundle for a code path +(`BigNumber.random`) that nothing calls. See [src/lib/cryptoStub.js](./src/lib/cryptoStub.js). + Two of the main features provided by rhtmlBuildUtils are to start the internal web server and to run the visual regression tests. These topics are covered in these subdocs: * [internal web server](./docs/internal_web_server.md) @@ -151,6 +193,8 @@ The top level tasks are those you will likely run as part of the widget build pr `rhtml testVisual_s` : just run the visual regression suite (skip the other steps, `rhtml serve` must already be running). +`rhtml reviewBaselines --from [--to ]` : build a local side-by-side review page for image snapshot baselines, at `.tmp/reviewBaselines/index.html`. GitHub's diff renderer gives up on a few hundred binary files, which is exactly the size of a regenerated baseline set — and reviewing the images is the real gate when accepting new baselines, since a rendering regression accepted there is invisible afterwards. Omit `--to` to compare the working tree against ``. Baselines reported as *identical* are worth looking at first: one that did not regenerate usually means its test errored before reaching the snapshot. + `rhtml lint` : this runs the eslint style checker on all the javascript files. Our settings are defined in [eslint.config.base.js](./eslint.config.base.js), which your widget repo's `eslint.config.js` re-exports. Which files are checked is decided by the `ignores` in that config rather than by this task, because eslint 10 has no `.eslintignore`. To run with auto fix run `rhtml lint --fix`. Note that this is also run as a git prepush hook so you will not be able to push code to git unless it passes the style checks. # Developing / Contributing diff --git a/eslint.config.base.js b/eslint.config.base.js index 8c84414..24ec6b9 100644 --- a/eslint.config.base.js +++ b/eslint.config.base.js @@ -49,11 +49,24 @@ const browserContextFiles = [ // the test run for the same reason. const copiedTemplateTests = ['src/tasks/*/assets/*.jest.test.js'] -// The experiment UI is browser ES modules (`import _ from 'lodash'`), bundled by esbuild. It parsed -// under the old .eslintrc only because eslint-config-standard set sourceType: 'module' for the whole -// project, including the CommonJS majority. eslint-plugin-n's recommended-script config is correctly -// CommonJS, so the genuinely-ESM files now say so for themselves. -const browserEsmFiles = ['src/tasks/experiment/assets/ui/**/*.js'] +// Browser ES modules (`import _ from 'lodash'`), bundled by esbuild -- as opposed to the node CommonJS +// that everything else here and in a widget repo is written in. These parsed under the old .eslintrc +// only because eslint-config-standard set sourceType: 'module' for the WHOLE project, including the +// CommonJS majority; eslint-plugin-n's recommended-script config is correctly CommonJS, so the +// genuinely-ESM files have to say so for themselves. +// +// NB the widget entries matter as much as this package's own. `theSrc/scripts` is the standardised home +// for a widget's source -- both widgetEntryPoint and widgetFactory point into it (see +// src/config/default.widget.config.js) -- and it is ALL browser ESM. Without these globs a widget repo +// adopting this config gets a parsing error on every one of its own source files. Globs that do not +// apply in a given repo simply match nothing, so one list serves both. +const browserEsmFiles = [ + // this package's own experiment UI + 'src/tasks/experiment/assets/ui/**/*.js', + // a widget repo's source, and any browser javascript it serves from the internal web server + 'theSrc/scripts/**/*.js', + 'theSrc/internal_www/js/**/*.js' +] module.exports = [ { ignores }, @@ -109,13 +122,22 @@ module.exports = [ 'no-extend-native': 'error', 'no-new-func': 'error', - // @stylistic's customize() defaults are close to standard but differ on these five. Set to - // standard's values so no existing file is reformatted. + // @stylistic's customize() defaults are close to standard but differ on these. Set to standard's + // values so no existing file is reformatted. '@stylistic/comma-dangle': ['error', 'never'], '@stylistic/space-before-function-paren': ['error', 'always'], '@stylistic/brace-style': ['error', '1tbs', { allowSingleLine: true }], '@stylistic/quote-props': ['error', 'as-needed'], + // NB customize() defaults this to 'before', but eslint-config-standard used 'after' -- i.e. a + // wrapped expression keeps the operator at the END of the line. Nothing on master happened to + // wrap an operator, so the mismatch stayed invisible until a file written under the old config + // was added, which then reported 13 errors for style that was previously correct. Ternaries keep + // the operator at the start, which is standard's own exception. + '@stylistic/operator-linebreak': ['error', 'after', { + overrides: { '?': 'before', ':': 'before', '|>': 'before' } + }], + // Not enabled by eslint-config-standard, and the tree mixes `x => ...` with `(x) => ...`. // Turning it on would be a reformat, so leave the existing mix alone. '@stylistic/arrow-parens': 'off', diff --git a/package-lock.json b/package-lock.json index 58a9a2e..9721787 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,6 +16,7 @@ "app-root-dir": "^1.0.2", "babel-jest": "^29.7.0", "bluebird": "^3.7.2", + "buffer": "^6.0.3", "chokidar": "^4.0.3", "connect": "^3.7.0", "connect-livereload": "^0.6.1", @@ -25,6 +26,7 @@ "eslint": "^10.8.0", "eslint-plugin-n": "^18.2.2", "eslint-plugin-promise": "^7.3.0", + "events": "^3.3.0", "fancy-log": "^1.3.3", "fast-glob": "^3.3.3", "fs-extra": "^9.1.0", @@ -46,8 +48,12 @@ "serve-index": "^1.9.1", "serve-static": "^1.14.1", "shelljs": "^0.8.4", + "stream-browserify": "^3.0.0", "yargs": "^15.4.1" }, + "bin": { + "rhtml": "bin/rhtml.js" + }, "engines": { "node": "^20.19.0 || ^22.13.0 || >=24" } @@ -3585,6 +3591,26 @@ "bare-path": "^3.0.0" } }, + "node_modules/base64-js": { + "version": "1.5.1", + "resolved": "https://registry.npmjs.org/base64-js/-/base64-js-1.5.1.tgz", + "integrity": "sha512-AKpaYlHn8t4SVbOHCy+b5+KKgvR4vrsD8vbvrbiQJps7fKDTkjkDry6ji0rUJjC0kzbNePLwzxq8iypo41qeWA==", + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/feross" + }, + { + "type": "patreon", + "url": "https://www.patreon.com/feross" + }, + { + "type": "consulting", + "url": "https://feross.org/support" + } + ], + "license": "MIT" + }, "node_modules/baseline-browser-mapping": { "version": "2.11.8", "resolved": "https://registry.npmjs.org/baseline-browser-mapping/-/baseline-browser-mapping-2.11.8.tgz", @@ -3777,6 +3803,30 @@ "node-int64": "^0.4.0" } }, + "node_modules/buffer": { + "version": "6.0.3", + "resolved": "https://registry.npmjs.org/buffer/-/buffer-6.0.3.tgz", + "integrity": "sha512-FTiCpNxtwiZZHEZbcbTIcZjERVICn9yq/pDFkTl95/AxzD1naBctN7YO68riM/gLSDY7sdrMby8hofADYuuqOA==", + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/feross" + }, + { + "type": "patreon", + "url": "https://www.patreon.com/feross" + }, + { + "type": "consulting", + "url": "https://feross.org/support" + } + ], + "license": "MIT", + "dependencies": { + "base64-js": "^1.3.1", + "ieee754": "^1.2.1" + } + }, "node_modules/buffer-crc32": { "version": "0.2.13", "resolved": "https://registry.npmjs.org/buffer-crc32/-/buffer-crc32-0.2.13.tgz", @@ -5153,6 +5203,15 @@ "node": ">= 0.6" } }, + "node_modules/events": { + "version": "3.3.0", + "resolved": "https://registry.npmjs.org/events/-/events-3.3.0.tgz", + "integrity": "sha512-mQw+2fkQbALzQ7V0MY0IqdnXNOeTtP4r0lN9z7AAawCXgqea7bDii20AYrIBrFd/Hx0M2Ocz6S111CaFkUcb0Q==", + "license": "MIT", + "engines": { + "node": ">=0.8.x" + } + }, "node_modules/events-universal": { "version": "1.0.1", "resolved": "https://registry.npmjs.org/events-universal/-/events-universal-1.0.1.tgz", @@ -5934,6 +5993,26 @@ "node": ">=0.8.0" } }, + "node_modules/ieee754": { + "version": "1.2.1", + "resolved": "https://registry.npmjs.org/ieee754/-/ieee754-1.2.1.tgz", + "integrity": "sha512-dcyqhDvX1C46lXZcVqCpK+FtMRQVdIMN6/Df5js2zouUsqG7I6sFxitIC+7KYK29KdXOLHdu9zL4sFnoVQnqaA==", + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/feross" + }, + { + "type": "patreon", + "url": "https://www.patreon.com/feross" + }, + { + "type": "consulting", + "url": "https://feross.org/support" + } + ], + "license": "BSD-3-Clause" + }, "node_modules/ignore": { "version": "5.3.2", "resolved": "https://registry.npmjs.org/ignore/-/ignore-5.3.2.tgz", @@ -8882,6 +8961,30 @@ "node": ">= 0.6" } }, + "node_modules/stream-browserify": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/stream-browserify/-/stream-browserify-3.0.0.tgz", + "integrity": "sha512-H73RAHsVBapbim0tU2JwwOiXUj+fikfiaoYAKHF3VJfA0pe2BCzkhAHBlLG6REzE+2WNZcxOXjK7lkso+9euLA==", + "license": "MIT", + "dependencies": { + "inherits": "~2.0.4", + "readable-stream": "^3.5.0" + } + }, + "node_modules/stream-browserify/node_modules/readable-stream": { + "version": "3.6.2", + "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-3.6.2.tgz", + "integrity": "sha512-9u/sniCrY3D5WdsERHzHE4G2YCXqoG5FTHUiCC4SIbr6XcLZBY05ya9EKjYek9O5xOAwjGq+1JdGBAS7Q9ScoA==", + "license": "MIT", + "dependencies": { + "inherits": "^2.0.3", + "string_decoder": "^1.1.1", + "util-deprecate": "^1.0.1" + }, + "engines": { + "node": ">= 6" + } + }, "node_modules/streamx": { "version": "2.28.0", "resolved": "https://registry.npmjs.org/streamx/-/streamx-2.28.0.tgz", diff --git a/package.json b/package.json index 5506d1d..2514743 100644 --- a/package.json +++ b/package.json @@ -25,6 +25,7 @@ "app-root-dir": "^1.0.2", "babel-jest": "^29.7.0", "bluebird": "^3.7.2", + "buffer": "^6.0.3", "chokidar": "^4.0.3", "connect": "^3.7.0", "connect-livereload": "^0.6.1", @@ -34,6 +35,7 @@ "eslint": "^10.8.0", "eslint-plugin-n": "^18.2.2", "eslint-plugin-promise": "^7.3.0", + "events": "^3.3.0", "fancy-log": "^1.3.3", "fast-glob": "^3.3.3", "fs-extra": "^9.1.0", @@ -55,6 +57,7 @@ "serve-index": "^1.9.1", "serve-static": "^1.14.1", "shelljs": "^0.8.4", + "stream-browserify": "^3.0.0", "yargs": "^15.4.1" }, "overrides": { diff --git a/src/cli.js b/src/cli.js index 664991d..9065f70 100644 --- a/src/cli.js +++ b/src/cli.js @@ -1,24 +1,21 @@ const colors = require('ansi-colors') -const { runTasks, knownTaskNames } = require('./index') +const { runTasks, knownTaskNames, widgetConfig } = require('./index') const { runTeardowns } = require('./lib/teardown') +const parseTaskNames = require('./lib/parseTaskNames') // Entry point for the `rhtml` binary. Replaces `gulp [...] [--flags]` with // `rhtml [...] [--flags]`. // -// NB task names are taken as the positional arguments and flags are left alone, because the tasks -// parse their own flags out of process.argv via yargs (--fix, --env, --branch, --port, -t, -u, ...). -// Deliberately not routed through a yargs command definition here: that would have to re-declare -// every flag of every task, and the task-local declarations are what the docs describe. -const parseTaskNames = (argv) => argv.filter(arg => !arg.startsWith('-')) - +// NB the flags themselves are deliberately NOT parsed here. Each task declares and reads its own flags +// out of process.argv via yargs (--fix, --env, --branch, --port, --from, -t, -u, ...). Routing them +// through a yargs command definition here would mean re-declaring every flag of every task, and the +// task-local declarations are what the docs describe. +// +// Which arguments are task names is decided by src/lib/parseTaskNames.js. const main = async () => { const requested = parseTaskNames(process.argv.slice(2)) const taskNames = requested.length ? requested : ['default'] - // NB required lazily: widgetConfig reads /build/config/widget.config.js at require - // time and throws if it is absent, and we want that to surface as a clear error after argv parsing - // rather than as a stack trace before the CLI has done anything. - const { widgetConfig } = require('./index') const disabledTasks = widgetConfig.disabledTasks || [] await runTasks({ taskNames, disabledTasks }) diff --git a/src/lib/compileES6.js b/src/lib/compileES6.js index d9db489..a573c6b 100644 --- a/src/lib/compileES6.js +++ b/src/lib/compileES6.js @@ -31,7 +31,29 @@ module.exports = ({ entryPointFile, destinationDirectory, minify = false, callba format: 'iife', logLevel: 'silent', // we surface errors and warnings ourselves, below inject: [path.join(__dirname, 'esbuildPolyfillShim.js')], - alias: { crypto: 'crypto-browserify' }, // browserify shimmed node builtins implicitly + // browserify shimmed node builtins implicitly; esbuild does not, so the ones actually reached by + // widget dependency graphs are mapped here. + // + // browserify shimmed node builtins implicitly; esbuild does not, so the ones widget dependency + // graphs actually reach are mapped here. + // + // NB `crypto` is stubbed, NOT mapped to crypto-browserify. See src/lib/cryptoStub.js for the full + // reasoning: browserify shipped no crypto at all for these widgets, and resolving it costs 616 KiB + // for a code path nothing calls. A widget that genuinely needs it opts back in via + // esbuildOptions: { alias: { crypto: 'crypto-browserify' } }. + // + // NB buffer/stream/events stay mapped even though the default crypto stub no longer needs them. + // They are what makes the crypto-browserify opt-in above actually resolve -- its tree (asn1.js, + // browserify-sign, safe-buffer, cipher-base, hash-base, readable-stream) requires all three, and + // without them the bundle fails with "Could not resolve buffer" / "Could not resolve stream". They + // are also the right answer for any widget code that requires them directly, and cost nothing when + // nothing does. + alias: { + crypto: path.join(__dirname, 'cryptoStub.js'), + buffer: 'buffer', + stream: 'stream-browserify', + events: 'events' + }, define: { 'process.env.NODE_ENV': JSON.stringify(minify ? 'production' : 'development'), global: 'window' diff --git a/src/lib/cryptoStub.js b/src/lib/cryptoStub.js new file mode 100644 index 0000000..500b197 --- /dev/null +++ b/src/lib/cryptoStub.js @@ -0,0 +1,42 @@ +// Stands in for node's `crypto` when esbuild bundles a widget. Aliased in src/lib/compileES6.js. +// +// Why a stub is the DEFAULT rather than crypto-browserify: +// +// bignumber.js@2 -- used by rhtmlCombinedScatter and rhtmlLabeledScatter -- reaches for crypto with +// +// if ( !cryptoObj ) try { cryptoObj = require('cry' + 'pto'); } catch (e) {} +// +// The string concatenation and the try/catch are deliberate: they stop a bundler statically resolving +// crypto, and browserify duly shipped none of it. esbuild constant-folds the concatenation, so it DOES +// resolve, and aliasing to crypto-browserify then pulled 616 KiB across 180 files (elliptic, four separate +// copies of bn.js, asn1.js, browserify-sign, diffie-hellman) into rhtmlCombinedScatter's bundle, taking it +// from 1651 to 2341 KiB -- all for a code path (BigNumber.random) that nothing calls. Stubbing reproduces +// what browserify shipped for years. +// +// A widget that genuinely needs crypto in the browser opts back in from its widget.config.js: +// +// esbuildOptions: { alias: { crypto: 'crypto-browserify' } } +// +// NB rhtmlPictographs is the known case: CacheService.js and SvgDefinitionManager.js call +// crypto.createHash for cache keys, so it must opt in when it moves off 7.2.3. +// +// NB the members below THROW rather than being absent, so a widget that needs crypto fails with an +// actionable message rather than "crypto.createHash is not a function". Everything else is undefined, +// which is what bignumber's own feature detection expects. +const unavailable = (name) => () => { + throw new Error( + `node's crypto.${name}() is not in this bundle. rhtmlBuildUtils stubs 'crypto' by default, because ` + + 'resolving it pulls ~600 KiB of crypto-browserify into the widget for a code path that is usually ' + + 'never called. If this widget really does need crypto in the browser, opt back in from ' + + 'build/config/widget.config.js with: esbuildOptions: { alias: { crypto: \'crypto-browserify\' } }' + ) +} + +module.exports = { + createHash: unavailable('createHash'), + createHmac: unavailable('createHmac'), + randomBytes: unavailable('randomBytes'), + randomFillSync: unavailable('randomFillSync'), + pbkdf2: unavailable('pbkdf2'), + pbkdf2Sync: unavailable('pbkdf2Sync') +} diff --git a/src/lib/parseTaskNames.jest.test.js b/src/lib/parseTaskNames.jest.test.js new file mode 100644 index 0000000..144a051 --- /dev/null +++ b/src/lib/parseTaskNames.jest.test.js @@ -0,0 +1,44 @@ +const parseTaskNames = require('./parseTaskNames') + +test('a single task', () => { + expect(parseTaskNames(['build'])).toEqual(['build']) +}) + +test('several tasks run in sequence', () => { + expect(parseTaskNames(['core', 'compileInternal'])).toEqual(['core', 'compileInternal']) +}) + +test('no arguments yields no tasks, so the caller can fall back to default', () => { + expect(parseTaskNames([])).toEqual([]) +}) + +test('an --opt=value flag is not a task', () => { + expect(parseTaskNames(['testVisual', '--env=local', '--branch=master'])).toEqual(['testVisual']) +}) + +// These are the regression. A space-separated flag value is a bare word, so filtering on "does not start +// with -" read it as a task name and failed with "unknown task 'HEAD'". Both forms below are used in +// anger: reviewBaselines takes `--from `, and rhtmlCombinedScatter's CI runs `-t "$TEST_FILTER"`. +test('a space separated --flag value is not a task', () => { + expect(parseTaskNames(['reviewBaselines', '--from', 'HEAD'])).toEqual(['reviewBaselines']) +}) + +test('a space separated short flag value is not a task', () => { + expect(parseTaskNames(['testVisual', '-t', 'someFilter'])).toEqual(['testVisual']) +}) + +test('several flag values after several tasks', () => { + expect(parseTaskNames(['reviewBaselines', '--from', 'abc123^', '--to', 'abc123'])) + .toEqual(['reviewBaselines']) +}) + +test('a boolean short flag with no value', () => { + expect(parseTaskNames(['testVisual_s', '-u'])).toEqual(['testVisual_s']) +}) + +// Flags before tasks are not supported: everything from the first flag on is flag territory, so the task +// name would be swallowed. Documented rather than fixed, because supporting it needs a per-flag arity +// table, which is exactly the re-declaration of every task's flags that the CLI avoids. +test('flags placed BEFORE the task names swallow them', () => { + expect(parseTaskNames(['--port', '9001', 'serve'])).toEqual([]) +}) diff --git a/src/lib/parseTaskNames.js b/src/lib/parseTaskNames.js new file mode 100644 index 0000000..8881eda --- /dev/null +++ b/src/lib/parseTaskNames.js @@ -0,0 +1,13 @@ +// Task names are the LEADING positional arguments of `rhtml ... [--flags]`, stopping at the first +// flag. Everything from the first flag onward belongs to the flags, which each task parses for itself. +// +// NB the stop-at-first-flag rule is the whole point. A space-separated flag VALUE is a bare word, so +// filtering on "does not start with -" reads the HEAD in `--from HEAD`, or the filter in +// `-t someFilter`, as a task name -- which fails with "unknown task 'HEAD'". gulp behaved the same way, +// taking task names first. +// +// Extracted from src/cli.js so it can be tested without executing the CLI, which calls process.exit. +module.exports = function parseTaskNames (argv) { + const firstFlag = argv.findIndex(arg => arg.startsWith('-')) + return (firstFlag === -1) ? argv : argv.slice(0, firstFlag) +} diff --git a/src/lib/renderExamplePageTest.helper.jest.test.js b/src/lib/renderExamplePageTest.helper.jest.test.js new file mode 100644 index 0000000..659f608 --- /dev/null +++ b/src/lib/renderExamplePageTest.helper.jest.test.js @@ -0,0 +1,139 @@ +const fs = require('fs-extra') +const os = require('os') +const path = require('path') + +// NB widgetConfig reads a build/config/widget.config from the consuming widget project, which does not +// exist in this repo, so it is stubbed. basePath is a real temp directory because these tests assert on +// the diagnostic png that testSnapshots writes when a comparison fails. +const mockBasePath = fs.mkdtempSync(path.join(os.tmpdir(), 'rbu-helper-')) + +jest.mock('./widgetConfig', () => ({ + basePath: mockBasePath, + snapshotTesting: { + snapshotDelay: 0, + snapshotDirectory: 'theSrc/test/snapshots', + env: 'local', + branch: 'master', + timeout: 1000, + pixelmatch: {} + }, + internalWebSettings: { + singleWidgetSnapshotSelector: '.widget', + statePreprocessor: x => x + } +})) + +const { testSnapshots } = require('./renderExamplePageTest.helper') + +const newSnapshotsDir = path.join(mockBasePath, 'theSrc/test/snapshots/local/master/new_snapshots') + +// A page whose $$ resolves `count` fake widgets, each screenshotting to a distinct buffer. +const fakePage = (count) => ({ + $$: async () => Array.from({ length: count }, (_unused, index) => ({ + screenshot: async () => Buffer.from(`image-${index}`) + })) +}) + +// Replaces the real jest-image-snapshot matcher so a comparison can be made to fail on demand without +// any actual image diffing, and so the identifier testSnapshots asks for can be asserted on. +// testSnapshots calls expect(image).toMatchImageSnapshot({...}). +let requestedIdentifiers = [] + +const matcherFailingFor = (failingIdentifiers) => { + requestedIdentifiers = [] + expect.extend({ + toMatchImageSnapshot (received, { customSnapshotIdentifier }) { + requestedIdentifiers.push(customSnapshotIdentifier) + const pass = !failingIdentifiers.includes(customSnapshotIdentifier) + return { pass, message: () => `stub matcher: ${customSnapshotIdentifier} did not match` } + } + }) +} + +beforeEach(() => fs.removeSync(newSnapshotsDir)) +afterAll(() => fs.removeSync(mockBasePath)) + +test('resolves when every snapshot matches', async () => { + matcherFailingFor([]) + + await expect(testSnapshots({ page: fakePage(1), testName: 'all good' })).resolves.toBeUndefined() +}) + +// This is the regression the whole fix is for. The catch used to swallow the matcher's throw, so a test +// whose images did not match reported PASS -- the job only went red via jest's aggregate +// snapshotState.unmatched count, and reading the per-test list led to the wrong conclusion. +test('REJECTS when a snapshot does not match, rather than reporting pass', async () => { + matcherFailingFor(['a_mismatch-snap']) + + await expect(testSnapshots({ page: fakePage(1), testName: 'a mismatch' })) + .rejects.toThrow(/1 snapshot\(s\) did not match: a_mismatch/) +}) + +test('names every failure, not just the first', async () => { + matcherFailingFor(['multi-one-snap', 'multi-three-snap']) + + const promise = testSnapshots({ + page: fakePage(3), + testName: 'multi', + snapshotNames: ['one', 'two', 'three'] + }) + + await expect(promise).rejects.toThrow(/2 snapshot\(s\) did not match: multi-one, multi-three/) +}) + +// Throwing from inside the loop would abort it, losing the diagnostic image for every widget after the +// first failure. Collecting and rethrowing at the end keeps the full set. +test('writes a diagnostic png for every failure, including ones after the first', async () => { + matcherFailingFor(['multi-one-snap', 'multi-three-snap']) + + await expect(testSnapshots({ + page: fakePage(3), + testName: 'multi', + snapshotNames: ['one', 'two', 'three'] + })).rejects.toThrow() + + expect(fs.readdirSync(newSnapshotsDir).sort()).toEqual(['multi-one-snap.png', 'multi-three-snap.png']) +}) + +// The previous implementation used the fs.writeFile callback form and never awaited it, so the process +// could exit before the diagnostic image reached disk. +test('the diagnostic png is on disk by the time the rejection surfaces', async () => { + matcherFailingFor(['sync_check-snap']) + + await expect(testSnapshots({ page: fakePage(1), testName: 'sync check' })).rejects.toThrow() + + const written = path.join(newSnapshotsDir, 'sync_check-snap.png') + expect(fs.existsSync(written)).toBe(true) + expect(fs.readFileSync(written).toString()).toBe('image-0') +}) + +test('writes nothing to new_snapshots when everything matches', async () => { + matcherFailingFor([]) + + await testSnapshots({ page: fakePage(2), testName: 'clean run', snapshotNames: ['a', 'b'] }) + + expect(fs.existsSync(newSnapshotsDir)).toBe(false) +}) + +// These two pin the baseline FILENAME, which is the contract with every committed baseline in every +// widget repo: they are all -snap.png. jest-image-snapshot v3 appended '-snap' to a custom +// identifier; v6 appends it only when no custom identifier is given +// (`customSnapshotIdentifier || `${defaultIdentifier}-snap``) and writes `${identifier}.png`. So under +// v6 the suffix has to come from here. Getting this wrong is silent and expensive: nothing matches the +// existing baselines, every test looks new, and a regeneration writes a parallel un-suffixed set while +// orphaning the old one -- which is exactly what happened on the first rhtmlCombinedScatter run. +test('asks for an identifier ending in -snap, so the baseline is -snap.png', async () => { + matcherFailingFor([]) + + await testSnapshots({ page: fakePage(1), testName: 'grid' }) + + expect(requestedIdentifiers).toEqual(['grid-snap']) +}) + +test('suffixes every widget on a multi-widget page', async () => { + matcherFailingFor([]) + + await testSnapshots({ page: fakePage(3), testName: 'multi', snapshotNames: ['one', 'two', 'three'] }) + + expect(requestedIdentifiers).toEqual(['multi-one-snap', 'multi-two-snap', 'multi-three-snap']) +}) diff --git a/src/lib/renderExamplePageTest.helper.js b/src/lib/renderExamplePageTest.helper.js index 98b325a..ce3d39f 100644 --- a/src/lib/renderExamplePageTest.helper.js +++ b/src/lib/renderExamplePageTest.helper.js @@ -149,6 +149,18 @@ const testSnapshots = async ({ page, testName, snapshotNames = null }) => { } } + // NB failures are COLLECTED here and rethrown once at the end, rather than swallowed or thrown + // immediately. Two separate reasons: + // + // * Swallowing them (what this did before) meant a test whose images did not match reported PASS. + // The job only went red via jest's aggregate snapshotState.unmatched count, so reading the + // per-test list led straight to the wrong conclusion about what had actually failed. + // * Throwing from inside the loop would abort it, so a multi-widget test would lose the diagnostic + // new_snapshots image for every widget after the first failure. + // + // Collecting keeps the full set of diagnostics while still failing the test that failed. + const failures = [] + await asyncForEach(widgets, async (widget, index) => { // NB puppeteer now declares screenshot() as resolving a Uint8Array. It still hands back a // Buffer in practice, but jest-image-snapshot passes this straight to pngjs, which calls @@ -157,8 +169,20 @@ const testSnapshots = async ({ page, testName, snapshotNames = null }) => { let image = Buffer.isBuffer(rawImage) ? rawImage : Buffer.from(rawImage) const snapshotName = getSnapshotName(index) try { - expect(image).toMatchImageSnapshot({ customSnapshotIdentifier: snapshotName }) + // NB the '-snap' suffix is applied HERE, and must be. jest-image-snapshot changed this between + // v3 and v6. Its createSnapshotIdentifier now reads: + // + // let snapshotIdentifier = customSnapshotIdentifier || `${defaultIdentifier}-snap` + // + // so '-snap' is appended only when NO custom identifier is given -- and the baseline file is + // `${snapshotIdentifier}.png`. v3 appended it either way, which is why every committed baseline + // in every widget repo is named -snap.png. Passing the bare name under v6 writes + // .png instead: nothing matches the existing baselines, every test looks new, and a + // regeneration silently produces a parallel un-suffixed set while orphaning all the old ones. + expect(image).toMatchImageSnapshot({ customSnapshotIdentifier: `${snapshotName}-snap` }) } catch (e) { + failures.push({ snapshotName, error: e }) + // Can't find group name so just put all new snapshots in same folder const snapshotDirectory = path.join( widgetConfig.basePath, @@ -167,12 +191,18 @@ const testSnapshots = async ({ page, testName, snapshotNames = null }) => { widgetConfig.snapshotTesting.branch ) const newSnapshotDir = path.join(snapshotDirectory, 'new_snapshots') - if (!fs.existsSync(newSnapshotDir)) fs.mkdirSync(newSnapshotDir) - fs.writeFile(path.join(newSnapshotDir, `${snapshotName}-snap.png`), image, 'binary', (err) => { - if (err) console.log('Error saving new image snapshot: ' + err) - }) + // NB synchronous: the previous fs.writeFile callback form was fire-and-forget, so the process + // could exit before the diagnostic image reached disk. + fs.mkdirpSync(newSnapshotDir) + fs.writeFileSync(path.join(newSnapshotDir, `${snapshotName}-snap.png`), image, 'binary') } }) + + if (failures.length) { + const names = failures.map(({ snapshotName }) => snapshotName).join(', ') + const detail = failures.map(({ error }) => error.message).join('\n\n') + throw new Error(`${failures.length} snapshot(s) did not match: ${names}\n\n${detail}`) + } } module.exports = { diff --git a/src/tasks/misc/clean.js b/src/tasks/misc/clean.js index 9a72e68..fd94e24 100644 --- a/src/tasks/misc/clean.js +++ b/src/tasks/misc/clean.js @@ -1,10 +1,22 @@ -const Promise = require('bluebird') -const fs = Promise.promisifyAll(require('fs-extra')) +const fs = require('fs-extra') +const path = require('path') +const { basePath } = require('../../lib/widgetConfig') + +// NB 'man' is deliberately NOT in this list. It holds roxygen output (man/*.Rd), which is TRACKED and +// which only the makeDocs task can regenerate -- and makeDocs shells out to R, then swallows its own +// failure so that a missing R install is not fatal. Deleting man/ here therefore destroyed tracked R +// documentation on every machine without R on PATH (every Windows dev, every CI runner) and left it +// destroyed, printing a "make docs failed" line and carrying on. +// +// The other entries are safe to delete because each is rebuilt by a step that cannot silently no-op: +// 'browser' and '.tmp' are build scratch, 'inst' comes from compileWidgetEntryPoint plus core, and 'R' +// from core's copy task. +const LOCATIONS_TO_DELETE = ['browser', 'inst', 'R', '.tmp'] module.exports = () => { - return function (done) { - const locationsToDelete = ['browser', 'inst', 'man', 'R', '.tmp'] - const deletePromises = locationsToDelete.map(function (location) { return fs.removeAsync(location) }) - Promise.all(deletePromises).then(function () { done() }) + return async function () { + // NB resolved against basePath rather than the process cwd, matching every other task. Under gulp + // these were cwd-relative and happened to work because gulp was always invoked from the widget root. + await Promise.all(LOCATIONS_TO_DELETE.map(location => fs.remove(path.join(basePath, location)))) } } diff --git a/src/tasks/misc/makeDocs.js b/src/tasks/misc/makeDocs.js index 197c37b..5b4a8c4 100644 --- a/src/tasks/misc/makeDocs.js +++ b/src/tasks/misc/makeDocs.js @@ -1,10 +1,22 @@ const shell = require('shelljs') +// NB Rscript, not `r`. littler (`r`) is not available on Windows and is uncommon elsewhere, whereas +// Rscript ships with every R install. The old command also used a bash herestring (<<<) and POSIX +// output redirects, neither of which cmd.exe can parse, so on Windows it could never succeed even with +// R installed. shelljs's own `silent` option replaces the redirects. +const COMMAND = 'Rscript -e "library(devtools); document()"' + module.exports = () => { return function (done) { - const commandString = 'r --no-save 2>/dev/null >/dev/null <<< "library(devtools); document()"' - const exitCode = shell.exec(commandString).code - if (exitCode !== 0) console.log(`make docs failed with code ${exitCode}. Command was '${commandString}'`) - done(null) // don't trigger failure (e.g. if R is not installed) + const exitCode = shell.exec(COMMAND, { silent: true }).code + + // Still non-fatal: R and devtools are genuinely optional for JS-only development, and that is why + // this task swallows its exit code. What changed is that `clean` no longer deletes man/, so a + // skipped makeDocs now leaves the committed documentation intact rather than leaving a hole. + if (exitCode !== 0) { + console.log(`make docs skipped: '${COMMAND}' exited ${exitCode}. ` + + 'This is not fatal -- R and devtools are optional. man/*.Rd is left as committed.') + } + done(null) } } diff --git a/src/tasks/snapshot/reviewBaselines/index.js b/src/tasks/snapshot/reviewBaselines/index.js new file mode 100644 index 0000000..8040a25 --- /dev/null +++ b/src/tasks/snapshot/reviewBaselines/index.js @@ -0,0 +1,278 @@ +// Builds a local side-by-side review page for image snapshot baselines. +// +// Why this exists: GitHub's diff renderer gives up on a few hundred binary +// files ("Unable to render code block"), which is exactly the size of a +// regenerated baseline set. Reviewing the images is the real gate when accepting +// new baselines -- a rendering regression accepted there is invisible afterwards +// -- so it needs to be possible outside the PR view. +// +// Usage: +// rhtml reviewBaselines --from HEAD compare working tree against HEAD +// rhtml reviewBaselines --from abc123^ --to abc123 compare a commit against its parent +// +// Output: .tmp/reviewBaselines/index.html + +const _ = require('lodash') +const fs = require('fs-extra') +const path = require('path') +const shell = require('shelljs') +const widgetConfig = require('../../../lib/widgetConfig') +const getCommandLineArgs = require('./parseCommandLineArguments') + +const WORKING_TREE = '(working tree)' + +module.exports = () => { + return function (done) { + const args = getCommandLineArgs() + + if (!args.from) { + return done(new Error( + 'reviewBaselines needs --from , the baselines to compare against.\n' + + ' rhtml reviewBaselines --from HEAD\n' + + ' rhtml reviewBaselines --from abc123^ --to abc123' + )) + } + + const { snapshotTesting, basePath } = widgetConfig + const env = args.env || snapshotTesting.env + const branch = args.branch || snapshotTesting.branch + const snapshotPath = [snapshotTesting.snapshotDirectory, env, branch].join('/') + + const outputDirectory = path.join(basePath, '.tmp', 'reviewBaselines') + fs.removeSync(outputDirectory) + fs.mkdirpSync(outputDirectory) + + // Guard against comparing a ref with itself, which would report every + // baseline as identical -- a silently wrong answer from a tool whose whole + // job is spotting differences. + if (args.to) { + const resolve = (ref) => shell.exec(`git rev-parse "${ref}"`, { cwd: basePath, silent: true }) + const fromSha = resolve(args.from) + const toSha = resolve(args.to) + if (fromSha.code === 0 && toSha.code === 0 && fromSha.stdout.trim() === toSha.stdout.trim()) { + return done(new Error( + `--from "${args.from}" and --to "${args.to}" both resolve to ${fromSha.stdout.trim().slice(0, 7)}, ` + + 'so there is nothing to compare.' + )) + } + } + + let from, to + try { + from = materialise({ ref: args.from, snapshotPath, outputDirectory, basePath, name: 'from' }) + to = args.to + ? materialise({ ref: args.to, snapshotPath, outputDirectory, basePath, name: 'to' }) + : { + root: path.join(basePath, ...snapshotPath.split('/')), + label: WORKING_TREE, + // index.html sits at /.tmp/reviewBaselines/, so the working + // tree is two levels up + relativeToOutput: `../../${snapshotPath}` + } + } catch (error) { + return done(error) + } + + const pairs = collectPairs({ from, to }) + if (!pairs.length) { + return done(new Error( + `No .png baselines found under ${snapshotPath} in either ref. ` + + 'Check --env and --branch: the path is //.' + )) + } + + const outputFile = path.join(outputDirectory, 'index.html') + fs.writeFileSync(outputFile, renderPage({ pairs, from, to, snapshotPath }), 'utf8') + + const counts = _.countBy(pairs, 'status') + console.log(`reviewBaselines: ${pairs.length} baseline(s) under ${snapshotPath}`) + console.log(` ${_.map(counts, (n, status) => `${n} ${status}`).join(', ')}`) + console.log(` open ${outputFile}`) + done() + } +} + +// Extract the snapshot tree at a ref into the output directory. Deliberately +// avoids `git archive | tar` -- the pipe is unreliable under shelljs on Windows, +// which shells out through cmd.exe. Writing the tar then extracting it works on +// both, since Windows has shipped bsdtar since 1803. +const materialise = ({ ref, snapshotPath, outputDirectory, basePath, name }) => { + const destination = path.join(outputDirectory, name) + const tarFile = path.join(outputDirectory, `${name}.tar`) + fs.mkdirpSync(destination) + + // The ref is quoted because shelljs shells through cmd.exe on Windows, where + // ^ is the escape character -- an unquoted "HEAD^" silently becomes "HEAD", + // which would compare a commit against itself and report no differences. + const archive = shell.exec( + `git archive --format=tar --output="${tarFile}" "${ref}" -- "${snapshotPath}"`, + { cwd: basePath, silent: true } + ) + if (archive.code !== 0) { + throw new Error(`Could not read ${snapshotPath} at "${ref}": ${archive.stderr.trim()}`) + } + + // Extract with cwd set to the destination and a RELATIVE path to the archive. + // GNU tar (which is what Git Bash provides on Windows) reads an absolute + // Windows path as an rsh-style host spec and fails with + // "Cannot connect to C: resolve failed". Relative paths avoid the drive letter + // and work under both GNU tar and the bsdtar that ships with Windows. + const extract = shell.exec(`tar -xf "../${path.basename(tarFile)}"`, { + cwd: destination, silent: true + }) + if (extract.code !== 0) { + throw new Error(`Could not extract the snapshot archive for "${ref}": ${extract.stderr.trim()}`) + } + fs.removeSync(tarFile) + + return { + root: path.join(destination, ...snapshotPath.split('/')), + label: ref, + relativeToOutput: `${name}/${snapshotPath}` + } +} + +const findPngs = (root) => { + if (!fs.existsSync(root)) { return [] } + const walk = (directory) => _.flatMap(fs.readdirSync(directory, { withFileTypes: true }), (entry) => { + const full = path.join(directory, entry.name) + if (entry.isDirectory()) { return walk(full) } + return entry.name.endsWith('.png') ? [path.relative(root, full).split(path.sep).join('/')] : [] + }) + return walk(root) +} + +const collectPairs = ({ from, to }) => { + const relativePaths = _.union(findPngs(from.root), findPngs(to.root)).sort() + + return relativePaths.map((relativePath) => { + const fromFile = path.join(from.root, ...relativePath.split('/')) + const toFile = path.join(to.root, ...relativePath.split('/')) + const hasFrom = fs.existsSync(fromFile) + const hasTo = fs.existsSync(toFile) + + let status + if (!hasFrom) { status = 'added' } else if (!hasTo) { status = 'removed' } else { + status = fs.readFileSync(fromFile).equals(fs.readFileSync(toFile)) ? 'identical' : 'changed' + } + + return { + relativePath, + status, + collection: relativePath.includes('/') ? relativePath.replace(/\/[^/]+$/, '') : '(root)', + name: relativePath.replace(/^.*\//, ''), + fromSrc: hasFrom ? `${from.relativeToOutput}/${relativePath}` : null, + toSrc: hasTo ? `${to.relativeToOutput}/${relativePath}` : null + } + }) +} + +const escapeHtml = (value) => String(value) + .replace(/&/g, '&').replace(//g, '>').replace(/"/g, '"') + +const LABELS = { + changed: 'changed', + added: 'new - no baseline in --from', + removed: 'removed', + identical: 'identical - not regenerated' +} + +const renderPage = ({ pairs, from, to, snapshotPath }) => { + const counts = _.countBy(pairs, 'status') + const summary = _(['changed', 'added', 'removed', 'identical']) + .filter((status) => counts[status]) + .map((status) => `${counts[status]} ${status}`) + .join(' · ') + + const body = _(pairs) + .groupBy('collection') + .map((items, collection) => { + const heading = `

${escapeHtml(collection)} ` + + `${_.map(_.countBy(items, 'status'), (n, s) => `${n} ${s}`).join(' ')}

` + return heading + items.map(renderPair).join('\n') + }) + .join('\n') + + return ` + +Baseline review: ${escapeHtml(from.label)} to ${escapeHtml(to.label)} + +

Baseline review — ${escapeHtml(snapshotPath)}

+

Left: ${escapeHtml(from.label)}. Right: ${escapeHtml(to.label)}. +Accepting these images makes them the definition of correct, so a regression accepted here is invisible afterwards. +Start with identical: a baseline that did not regenerate usually means its test errored before reaching the snapshot.

+
+ ${summary} + + + + + + +
+${body} + +` +} + +const renderPair = ({ relativePath, status, name, fromSrc, toSrc }) => { + const side = (src, caption, missingText) => '
' + caption + '
' + + (src + ? `${escapeHtml(relativePath)}` + : `
${missingText}
`) + '
' + + return `
` + + `
${escapeHtml(name)}` + + `${LABELS[status]}
` + + '
' + + side(fromSrc, 'before', 'no baseline') + + side(toSrc, 'after', 'removed') + + '
' +} diff --git a/src/tasks/snapshot/reviewBaselines/parseCommandLineArguments.js b/src/tasks/snapshot/reviewBaselines/parseCommandLineArguments.js new file mode 100644 index 0000000..fcb878f --- /dev/null +++ b/src/tasks/snapshot/reviewBaselines/parseCommandLineArguments.js @@ -0,0 +1,33 @@ +// NB avoid defaults for env and branch: if a default is specified then the arg +// object contains the value regardless of whether it was given on the command +// line, which would override the widget's own config. Same convention as +// takeSnapshotsForEachTestDefinition. + +const yargs = require('yargs') + +module.exports = () => { + yargs.option('from', { + alias: 'f', + string: true, + describe: 'git ref holding the baselines to compare against, e.g. HEAD or a commit sha' + }) + // NB deliberately no short alias. `-t` would be the obvious one, but yargs is a shared singleton + // across the task modules and -t already means testNamePattern in jestSpecTests and + // takeSnapshotsForEachTestDefinition. Declaring it twice with different meanings is a trap for + // whoever hits it. + yargs.option('to', { + string: true, + describe: 'git ref holding the new baselines. Omit to use the working tree' + }) + yargs.option('env', { + alias: 'e', + string: true, + describe: 'which snapshot env to review (defaults to the widget config)' + }) + yargs.option('branch', { + alias: 'b', + string: true, + describe: 'which snapshot branch to review (defaults to the widget config)' + }) + return yargs.parse() +} diff --git a/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/index.js b/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/index.js index 80c8479..cff14e3 100644 --- a/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/index.js +++ b/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/index.js @@ -39,6 +39,7 @@ module.exports = () => { // connect now registers its server with src/lib/teardown.js, so the runner closes it and owns the // exit code, and this task is free to appear anywhere in a sequence. return shell.exec(command, { async: true }, (exitCode) => { + removePassThroughConfigFile({ widgetConfig }) const error = (exitCode === 0) ? null : new Error(`${command} failed with code ${exitCode}`) done(error) }) @@ -67,6 +68,11 @@ const getCommandString = ({ testRoots, jestPath, args }) => { return `"${jestPath}" ${roots} ${testFilePattern} ${acceptNewSnapshots} ${updateSnapshots} ${testNamePattern}` } +// The only reason this file exists is to carry command line values across a process boundary: this task +// shells out to jest, and jest propagates no arguments to the workers that actually read widgetConfig. +const passThroughConfigPath = ({ widgetConfig }) => + path.join(widgetConfig.basePath, '.tmp', 'snapshot_dynamic_config.json') + const writePassThroughConfigFile = ({ widgetConfig, args }) => { const dynamicSnapshotConfig = _.pick(args, ['branch', 'env', 'snapshotDirectory']) @@ -79,6 +85,20 @@ const writePassThroughConfigFile = ({ widgetConfig, args }) => { } const configString = JSON.stringify(dynamicSnapshotConfig, {}, 2) - fs.writeFileSync(path.join(widgetConfig.basePath, '.tmp', 'snapshot_dynamic_config.json'), configString, 'utf8') + fs.writeFileSync(passThroughConfigPath({ widgetConfig }), configString, 'utf8') if (ECHO_PASSTHROUGH_CONFIG) { console.log(`snapshot dynamic config: ${configString}`) } } + +// NB removed as soon as the jest run finishes, because widgetConfig merges this file at HIGHER precedence +// than the widget's own build/config/widget.config.js. Left behind, it silently reconfigures every LATER +// task that reads widgetConfig -- so a filtered run like +// +// rhtml testVisual --env=local --branch=probe --snapshotDirectory=.tmp/probe +// +// left `rhtml reviewBaselines` pointing at .tmp/probe rather than the real snapshot tree, which is exactly +// when you need reviewBaselines to be right. It is a process-boundary hack, not ambient state, so it +// should not outlive the process it was written for. +const removePassThroughConfigFile = ({ widgetConfig }) => { + // force: true so a run that never got as far as writing the file does not fail on the way out. + fs.rmSync(passThroughConfigPath({ widgetConfig }), { force: true }) +} diff --git a/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/parseCommandLineArguments.js b/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/parseCommandLineArguments.js index 35d0d28..7000605 100644 --- a/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/parseCommandLineArguments.js +++ b/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/parseCommandLineArguments.js @@ -5,22 +5,32 @@ const yargs = require('yargs') module.exports = () => { + // NB defaults to FALSE so that a snapshot with no baseline FAILS instead of being written and + // passed. When true, this appends --ci=0 to the jest command, which makes jest compute + // updateSnapshot: 'new' (jest-config/build/normalize.js), so jest-image-snapshot writes the missing + // baseline and the test goes green. Combined with baselines only leaving CI on an explicit + // regeneration, a newly added test could look green forever while never being regression-tested at + // all. Pass --acceptNewSnapshots to opt back in when bootstrapping a new suite. yargs.option('acceptNewSnapshots', { alias: 'a', - describe: 'accept new snapshots', + describe: 'write and pass snapshots that have no baseline, instead of failing', boolean: true, - default: true + default: false }) yargs.option('branch', { alias: 'b', string: true, describe: 'which branch are we testing (used to choose snapshot set)' }) + // NB no `choices` whitelist. It previously allowed only 'local' and 'travis', so a CI environment + // could not be named after the system actually running it -- rhtmlCombinedScatter had to bypass the + // flag entirely by setting snapshotTesting.env in its own widget.config.js, which works only because + // options without defaults are absent from the parsed args. The value is just a directory name under + // snapshotDirectory, so any string is valid. Travis has not been in use for years. yargs.option('env', { alias: 'e', string: true, - describe: 'which env are we testing (used to choose snapshot set)', - choices: ['local', 'travis'] + describe: 'which env are we testing, e.g. local or ci (chooses the snapshot set)' }) yargs.option('headless', { alias: 'h',