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(package): update spdy v3.4.1...4.0.0 (assertion error) (#1491) #1563

Merged
merged 2 commits into from
Dec 5, 2018

Conversation

odinho
Copy link
Contributor

@odinho odinho commented Nov 8, 2018

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

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.

Sorry, something went wrong.

@jsf-clabot
Copy link

jsf-clabot commented Nov 8, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@alexander-akait alexander-akait left a 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 😞

@odinho
Copy link
Contributor Author

odinho commented Nov 8, 2018

Interesting. Seems to both error and fail/crash (/Users/travis/.travis/job_stages: line 104: 3028 Segmentation fault: 11 npm run $SCRIPT) on Mac OS X. That is worrying.

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 master are having have been resolved.

@odinho
Copy link
Contributor Author

odinho commented Nov 8, 2018

If anything #1543 looks suspicious. It was added to master with its Travis check failing. Both commits since then have failed. The commit before was cancelled, so it actually never got to run a Travis build (but it was also only metadata update).

@alexander-akait
Copy link
Member

Can we fix it here?

@odinho
Copy link
Contributor Author

odinho commented Nov 9, 2018

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

@alexander-akait
Copy link
Member

No cache setup for this package to avoid same problem

@odinho odinho force-pushed the upgrade_spdy_4.0_to_fix_bug branch from 79eb0f3 to 76b07cc Compare December 4, 2018 15:28
@codecov
Copy link

codecov bot commented Dec 4, 2018

Codecov Report

Merging #1563 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fe82de...8c9944a. Read the comment docs.

@odinho
Copy link
Contributor Author

odinho commented Dec 4, 2018

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).

@alexander-akait
Copy link
Member

@odinho very thanks for helping, let's disable test and put comment with link on issue 👍

@odinho
Copy link
Contributor Author

odinho commented Dec 4, 2018

@odinho very thanks for helping, let's disable test and put comment with link on issue

I've added #1588

@alexander-akait
Copy link
Member

alexander-akait commented Dec 5, 2018

@odinho do we have way testing this?

@odinho
Copy link
Contributor Author

odinho commented Dec 5, 2018

@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)

@alexander-akait
Copy link
Member

@odinho okay, let's merge this

This was referenced Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants