fix: Handle the case when staged-git-files errors properly

[okonet]

Since the implementation was already merged in #255, this PR only adds a test. It will trigger semantic-release to actually release the new version.

Closes #264

[codecov]

Codecov Report

Merging #267 into master will increase coverage by 0.61%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
+ Coverage    89.5%   90.12%   +0.61%     
==========================================
  Files          10       10              
  Lines         162      162              
  Branches       23       23              
==========================================
+ Hits          145      146       +1     
+ Misses         16       15       -1     
  Partials        1        1

Impacted Files	Coverage Δ
src/runAll.js	77.77% <0%> (+3.7%)	⬆️

---

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 18c3d26...cf4ef0f. Read the comment docs.

[sudo-suhas]

Having to mock sgf could prove a problem when we want to add tests for all scenarios. This is what I would suggest:

  it('should reject if staged-git-files returns an error', () => {
    expect.assertions(1)
    return runAll(packageJson, getConfig({ gitDir: os.tmpdir() })).catch(err => {
      expect(err.message).toMatch(/fatal: Not a git repository/)
    })
  })

Do you know if there is a reliable way to get a temporary directory? I am using os.tmpdir() but would prefer to get a tmp directory for lint-staged itself.

[okonet]

I like your suggestion although I think we still will need to test for different scenarios and it is easier to do with the mock. Why do you think it's problematic?

BTW what about https://github.com/sindresorhus/os-tmpdir?

[sudo-suhas]

Why do you think it's problematic?

Take runScript for example. I don't like having to split my tests across files just for mocks. Not entirely sure if that will be relevant for this, just a little bit of intuition.

BTW what about https://github.com/sindresorhus/os-tmpdir?

This is deprecated, and a ponyfill for os.tmpdir()(what I am using already) which Sindre Sorhus recommends for node 4+.

[okonet]

Take runScript for example.
I don't like having to split my tests across files just for mocks.

Can you be more specific?

Not entirely sure if that will be relevant for this, just a little bit of intuition.

Since this is a bug fix for a blocker, I wouldn't spend much time here to make it right. We can always refactor later.

[sudo-suhas]

Can you be more specific?

We have 3 files for testing runScript:

• runScript.spec.js
• runScript-mock-findBin.spec.js
• runScript-mock-pMap.spec.js

If jest.doMock was working as expected, we could have these in a single file. And as we observed with the blinking test, there are weird bugs which can be introduced by mocks. So I prefer to keep mocks as the last resort. But you are right of course, we can refactor later.