Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix infer electron test introduced in 7.5.0 #441

Closed
malept opened this issue Aug 5, 2016 · 11 comments · Fixed by #445
Closed

Fix infer electron test introduced in 7.5.0 #441

malept opened this issue Aug 5, 2016 · 11 comments · Fixed by #445
Labels
Milestone

Comments

@malept
Copy link
Member

malept commented Aug 5, 2016

Since #435 was merged, tests for Node 4 in all host platforms intermittently raise an exception at the new infer Electron test.

Examples:

CC: @zeke

@malept malept added the bug 🐛 label Aug 5, 2016
@malept malept added this to the 7.5.1 milestone Aug 5, 2016
@zeke
Copy link
Contributor

zeke commented Aug 5, 2016

ack. investigating.

malept added a commit that referenced this issue Aug 6, 2016
@malept
Copy link
Member Author

malept commented Aug 6, 2016

I took a couple of minutes to look into it this morning. Here's how I reliably reproduced the problem:

  1. Switch to Node 4 environment (I used nodeenv)
  2. Take an existing checkout of electron-packager (<= 7.4.0) and run git pull && npm install
  3. Run node test/basic.js, not npm test

This intermittent error may have actually been an indicator for the problem in #439, but I'll need to see what CI says about that. In the meantime, I committed bcc7cac which at least gives a better exception.

@malept malept removed this from the 7.5.1 milestone Aug 6, 2016
@malept
Copy link
Member Author

malept commented Aug 6, 2016

😞 still happening: https://travis-ci.org/electron-userland/electron-packager/jobs/150300592#L273

I'm dropping it from the 7.5.1 milestone but I'd still like this to get fixed ASAP.

@zeke
Copy link
Contributor

zeke commented Aug 11, 2016

Following these steps:

n 4.4.7
git checkout tags/v7.4.0
rm -rf node_modules
npm i
node test/basic.js

I have run the tests several times and am not able to produce an error.

@malept
Copy link
Member Author

malept commented Aug 11, 2016

If I've learned anything from debugging intermittent test failures in general, it's that running a test in a testsuite vs. a single file has a tendency to be different, if a) your test runner does not isolate your tests properly, or b) you don't write your setup/teardown functions to isolate your tests properly.

My suspicion is that the failure will happen more often when running npm test rather than node test/basic.js.

@zeke zeke closed this as completed in #445 Aug 11, 2016
@malept
Copy link
Member Author

malept commented Aug 11, 2016

I don't consider this fixed (because it still happens intermittently), so I'm reopening.

@malept malept reopened this Aug 11, 2016
malept added a commit that referenced this issue Aug 12, 2016
It's hindering the resolution of #441.
@malept
Copy link
Member Author

malept commented Aug 12, 2016

Regarding the reproduction steps in #441 (comment) - they reliably reproduced the error until after bcc7cac was committed. Now it's just an intermittent error.

To assist in debugging this issue, I removed the use of tap-spec, which I suspect is hiding the real reason why it's failing.

@malept
Copy link
Member Author

malept commented Aug 12, 2016

Well, that's significantly more helpful:

https://travis-ci.org/electron-userland/electron-packager/jobs/151815017#L252-L283

@kevinsawicki
Copy link
Contributor

Can this be closed now? It doesn't seem to be happening once #452 was merged with the npm install --no-bin-links change.

@malept
Copy link
Member Author

malept commented Aug 22, 2016

Sounds good.

@malept malept closed this as completed Aug 22, 2016
@zeke
Copy link
Contributor

zeke commented Aug 22, 2016

Thanks for the help getting this fixed.

@malept malept added this to the 7.6.0 milestone Aug 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants