-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(package): update spdy
v3.4.1...4.0.0 (assertion error) (#1491)
#1563
fix(package): update spdy
v3.4.1...4.0.0 (assertion error) (#1491)
#1563
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! Looks tests failed 😞
Interesting. Seems to both error and fail/crash ( Seems like same errors are on master branch though. Only there Linux dies in the same way before Mac OS X gets to run. So I'll just re-run it later when whatever problems Travis or |
If anything #1543 looks suspicious. It was added to |
Can we fix it here? |
Maybe remove all caches? https://docs.travis-ci.com/user/common-build-problems/#segmentation-faults-from-the-language-interpreter-ruby-python-php-nodejs-etc I have tried some runs with what looked like the culprit removed (#1564) , but it still crashes (only stable though). So if you could kick Travis a bit, remove its caches, that might fix it? Howto clear cache from webui: https://docs.travis-ci.com/user/caching#clearing-caches |
No cache setup for this package to avoid same problem |
79eb0f3
to
76b07cc
Compare
Codecov Report
@@ Coverage Diff @@
## master #1563 +/- ##
=======================================
Coverage 74.02% 74.02%
=======================================
Files 10 10
Lines 670 670
=======================================
Hits 496 496
Misses 174 174 Continue to review full report at Codecov.
|
I've opened a bug with Node.js, hoping they can figure it out. nodejs/node#24835 An alternative (that I've been thinking of), is to change your test to test Node 10 instead of Node/stable (which now is Node 11.3.0, but this does not seem stable :P). |
@odinho very thanks for helping, let's disable test and put comment with link on issue 👍 |
@odinho do we have way testing this? |
It is an upstream bug, which is tricky to test in a good way downstream. That bug also only surfaced because Chromium fixed something on their side if I understand correctly. So I'm unsure how that would play out. Something that could have caught it would be a stress-test where a browser + the dev server is interacting for a long time (say 10 mins) with lots of different changes. Normally, updated version of dependencies won't really get their own tests. Even though having some test that would be able to find such issues would of course be helpful. (Though it'd be prone to noise and flaking, and it would be very little help in actually diagnosing the issues) |
@odinho okay, let's merge this |
For Bugs and Features; did you add new tests?
No. Upstream issue.
Motivation / Use-Case
Breaking Changes
No breaking change. Changelog for upstream:
Additional Info
No.