don't cache dependencies installed with file: protocol - add unit test #2443
Conversation
| const fixturesLoc = path.join(__dirname, '..', '..', 'fixtures', 'install'); | ||
| const compLoc = path.join(fixturesLoc, 'install-file-without-cache', 'comp', 'index.js'); | ||
|
|
||
| return fs.writeFile(compLoc,'foo\n').then(() => |
There was a problem hiding this comment.
No reason to mix promises and async/await.
await fs.writeFile would work here, too
There was a problem hiding this comment.
Even better! I just followed the style of most tests returning the first promise (mostly return runInstall).
| await fs.writeFile(compLoc, 'bar\n'); | ||
| await runInstall({noLockfile: true}, 'install-file-without-cache'); | ||
| assert.equal( | ||
| await fs.readFile(path.join(config.cwd, 'node_modules', 'comp', 'index.js')), |
There was a problem hiding this comment.
I think here you still read from the first install.
config.cwd comes as argument to runInstall.
Instead make this check inside the runInstall callback and double check that we are not testing the same location.
| await runInstall({noLockfile: true}, 'install-file-without-cache'); | ||
| assert.equal( | ||
| await fs.readFile(path.join(config.cwd, 'node_modules', 'comp', 'index.js')), | ||
| 'bar\n', |
There was a problem hiding this comment.
We can't merge a failing test, once you make sure it works correctly, can you switch the last assert to the opposite so that this test described the current behavior and add a link to the issue?
So that it would be easy to switch the assert when working on the fix.
|
@bestander I changed the unit test... an now I'm surprised. It passes 😮 Was this fixed already? It doesn't seem so... when I make a manual test with my local version |
|
Ha ha, probably not. |
|
@bestander I updated the unit test so it uses |
|
Thanks, I'll have a look in the next couple of days
…On Tue, 24 Jan 2017 at 21:07, Donald Pipowitch ***@***.***> wrote:
@bestander <https://github.com/bestander> I updated the unit test so it
uses Install. I used notEqual in the last assertion and linked to this
pull request to have a "working" unit test. Thank you very much for your
help so far.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2443 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACBdWGDGkQb8sjWTxl2Qib5CzOvdsR5Bks5rVmf3gaJpZM4Linri>
.
|
|
Thanks, that will help to keep track of it. |
|
Thank you a lot. Yeah, I'll try to fix the bug with another PR. |
|
:( the PR title is a bit misleading... |
donaldpipowitch
changed the title
|
Yes. I changed that. I'm sorry. In the beginning thought this would be an ongoing PR, but that idea was changed to two separate PRs: the first PR just adds a unit test, the second PR will fix it. |
…g#2165 (yarnpkg#2443) * added failing unit test for yarnpkg#2165 * fixed unit test * updated unit test
|
test for this was merged, but what's the status on the actual fix? its been mentioned in previous issues about the pain points of caching |
|
As said here (#2165 (comment)) I couldn't start another PR and AFAIK no one else did. |
Summary
I try to fix #2165. For now I just added a failing unit test. I've spoken with @bestander about this.
Test plan
An example is described here: #2165. And this PR contains a failing unit test. I try to install a dependency from a local file with content
foo\n. Than I change the content tobar\nand install it again. It should now bebar\n, but it is stillfoo\n@bestander I'm not sure this unit test works as expected. It looks like all tests assert only in the cache directory (?) given with
config.cwd. In my fixtures folder no dependency is added which I could check directly. Could you review the test?Is there any way to get a nicer test output when I only run one test? I do
$ npm run -s test-only -- -t 'without cache', but the output is quite verbose (e.g. contains a lot ofUnhandled promise rejectionerrors which seems to result from skipping the other tests).