pnpm / supi

Fast, disk space efficient installation engine. Used by pnpm
MIT License
24 stars 5 forks source link

fix: store path check should do a path-comparison, not a string comparison #39

Closed nickpape closed 6 years ago

nickpape commented 6 years ago

Should be doing a path based check rather than a strict string comparison

nickpape commented 6 years ago

Related issue: https://github.com/pnpm/pnpm/issues/996

zkochan commented 6 years ago

It failed because of the commit message format. It should be something like fix: ...

Have you tested if it fixes the issue? A test can cover this case as we run tests on Appveyor

nickpape commented 6 years ago

It definitely seems to fix the issue. I can look into adding a test case for it as well.

zkochan commented 6 years ago

cool, it should be really a simple test. Two runs of await install(). The second time the store param should be changed so that the disk letter has a different case.

nickpape commented 6 years ago

Okay, I will need to change the test I wrote! How will this test work on Linux machines? E.g. they don't have disk drives to change the path based on... Are there any Windows CI builds?

zkochan commented 6 years ago

I was thinking about something like

test('ignores drive case in store path', async (t: tape.Test) => {
  const project = prepare(t)
  const opts = await testDefaults()
  await installPkgs(['rimraf@2.5.1'], opts)

  await install(await testDefaults(), {...opts, store: makeFirstLetterSmall(store)})

  t.pass('Install did not fail')
})
zkochan commented 6 years ago

And it is enough to run it only on Windows. So the test can start like

test('ignores drive case in store path', async (t: tape.Test) => {
  if (!isWindows()) return
zkochan commented 6 years ago

Here's an existing test that checks whether the UNEXPECTED_STORE error is thrown: https://github.com/pnpm/supi/blob/master/test/install/misc.ts#L696-L707

This new one can be similar and placed next to it