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

update devDependencies and fix discovered issues #1318

Closed
wants to merge 1 commit into from

Conversation

jstewmon
Copy link

@jstewmon jstewmon commented Apr 7, 2019

  • context.onerror destorys the request socket if it is called after
    headers have been sent to complete the request
  • context.onerror now throws TypeError if passed a non-Error to align
    with application.onerror
  • fixed lint errors
    • remove usages of deprecated assertions
    • removed space between async and opening parens
  • replace calls to server.listen by passing app.callback() to supertest
    so that tests do not leave open event handlers
  • replaced callback completion test with async tests in cases where the
    tests failed due to callbacks being called in unexpected order

- context.onerror destorys the request socket if it is called after
  headers have been sent to complete the request
- context.onerror now throws TypeError if passed a non-Error to align
  with application.onerror
- fixed lint errors
  - remove usages of deprecated assertions
  - removed space between async and opening parens
- replace calls to server.listen by passing app.callback() to supertest
  so that tests do not leave open event handlers
- replaced callback completion test with async tests in cases where the
  tests failed due to callbacks being called in unexpected order
@jstewmon
Copy link
Author

jstewmon commented Apr 7, 2019

This one is a bit of a doozie - I started out wanting to address an issue when the body is a stream (which I'll follow up with in another issue), but I was distracted by all the deprecation warnings, and noticed the potential for some issues. Updating jest uncovered some issues with the test implementations (and one with the app), which are now fixed. :-)

@jstewmon
Copy link
Author

jstewmon commented Apr 7, 2019

node 7 is failing the deepStrictEqual check for the context query... I struggled to get that check passing locally w node 10 - I guess the prototype comparison fails when expected is {}. I found that using jest's expect(ctx.query).toEqual({}) passed too, but I tried to stick with the builtin assert methods, since there's no other use of expect...

Is it desirable to continue testing on node 7? If so, any suggestion on which way to fix the failure?

@dead-horse
Copy link
Member

can you separate it into different PRs to help us review your code ?

@jstewmon
Copy link
Author

jstewmon commented Apr 18, 2019

@dead-horse I think all these changes are strictly necessary to remove depreciation warnings and fix the tests after updating the deps. I suppose I could separate the depreciation updates, which is the source of most what you’d consider noise if that’s what you’re looking for?

Copy link

@anlexN anlexN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it is ok, please merge. when to merge?

Copy link
Member

@3imed-jaberi 3imed-jaberi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jstewmon
I don't like to be this person but I think should close this PR because it's outdated.

@dead-horse @niftylettuce @miwnwski @jonathanong ...

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.

5 participants