opensearch-project / oui

OpenSearch UI Framework
Apache License 2.0
35 stars 69 forks source link

[BUG] `scripts/test-staged.js` raise error when commit only include a Delete operation #1071

Open BigSamu opened 11 months ago

BigSamu commented 11 months ago

Describe the bug

Reviewing the base code, I realised that when you commit new changes, scripts/test-staged.js only considers files that are Added, Copied, Modified or Renamed but never a Deleted. When trying to commit a change that only has a Delete, the variable stagedFiles is an empty list (['']) and then the testing command from jest with flag --findRelatedTests complains because no location is given.

To Reproduce Steps to reproduce the behaviour:

  1. Add a txt file in the root folder with some text (e.g: Hello World)
  2. Add Added changes in the staging area and the commit (git add . and git -sm "adding random txt file)
  3. After the previous commit, Delete the txt file created.
  4. Add Delete changes in the staging area and the commit (git add . and git -sm "adding random txt file)
  5. See error.

Expected behavior

When committing you should not see an error from jest as described below

Error Message

Here below is the error I get:

oui git:(docs/modal-width) ✗ git commit -sm "removing file modal_width.js"
yarn run v1.22.19
$ yarn tsc --noEmit && yarn lint-es && yarn lint-sass
$ '/Users/samuel/Project Code/OpenSearch_Initiative_Programme/oui/node_modules/.bin/tsc' --noEmit
$ eslint --cache "{src,src-docs}/**/*.{ts,tsx,js}" --max-warnings 0
$ sass-lint -v --max-warnings 0
:sparkles:  Done in 8.29s.
yarn run v1.22.19
$ cross-env NODE_ENV=test jest --config ./scripts/jest/config.json --findRelatedTests
Usage: jest [--config=<pathToConfigFile>] [TestPathPattern]

Options:
    ...<Set of Options>

Documentation: https://jestjs.io/

The --findRelatedTests option requires file paths to be specified.
Example usage: jest --findRelatedTests ./src/source.js ./src/index.js.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
node:child_process:960
    throw err;
    ^

Error: Command failed: yarn test-unit --findRelatedTests 
    at checkExecSyncError (node:child_process:885:11)
    at execSync (node:child_process:957:15)
    at Object.<anonymous> (/Users/samuel/Project Code/OpenSearch_Initiative_Programme/oui/scripts/test-staged.js:20:1)
    at Module._compile (node:internal/modules/cjs/loader:1254:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
    at Module.load (node:internal/modules/cjs/loader:1117:32)
    at Module._load (node:internal/modules/cjs/loader:958:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:23:47 {
  status: 1,
  signal: null,
  output: [ null, null, null ],
  pid: 91528,
  stdout: null,
  stderr: null
}

Node.js v18.16.0
pre-commit: 
pre-commit: We've failed to pass the specified git pre-commit hooks as the `test-staged`
pre-commit: hook returned an exit code (1). If you're feeling adventurous you can
pre-commit: skip the git pre-commit hooks by adding the following flags to your commit:
pre-commit: 
pre-commit:   git commit -n (or --no-verify)
pre-commit: 
pre-commit: This is ill-advised since the commit is broken.
pre-commit:

Host/Environment (please complete the following information):

BSFishy commented 11 months ago

Most likely fix is just gating the execSync behind a "if the list isn't empty"

Also, would be nice to add D to the diff filter: https://git-scm.com/docs/git-diff#Documentation/git-diff.txt---diff-filterACDMRTUXB82308203

willie-hung commented 11 months ago

I would like to work on this. Thanks!