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

Superagent downgrading #558

Merged
merged 4 commits into from Mar 15, 2019
Merged

Superagent downgrading #558

merged 4 commits into from Mar 15, 2019

Conversation

rimiti
Copy link
Contributor

@rimiti rimiti commented Mar 13, 2019

Hello,

To quickly unblock the situation, I propose to downgrade the superagent version.

Details of this PR:

  • Test included to assert using promises (fail with superagent v4.x).
  • Superagent downgraded to "^3.8.3".

Related issues:

Current investigation:
The line causing this promise issue in superagent: https://github.com/visionmedia/superagent/blob/master/lib/request-base.js#L245

If this line is commented, supertest works fine with superagent v4.

Note: I am very busy to investigate for now, if anyone has time to work on a new pull request (regarding the safe update of superagent), it would be great. Otherwise I will do it as soon as I have time;

@rimiti rimiti changed the title Fixing promise usage Superagent downgrading Mar 14, 2019
@coveralls
Copy link

coveralls commented Mar 14, 2019

Pull Request Test Coverage Report for Build 395

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.297%

Totals Coverage Status
Change from base Build 389: 0.0%
Covered Lines: 137
Relevant Lines: 141

💛 - Coveralls

@rimiti
Copy link
Contributor Author

rimiti commented Mar 14, 2019

@gjohnson What do you think about that? 🤓

Copy link
Contributor

@gjohnson gjohnson left a comment

Choose a reason for hiding this comment

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

I suppose this is really the only path forward. My bad on not catching this last go around. In the next-release we should make sure we use a tag in npm or something so it's flagged as unstable.

@gjohnson
Copy link
Contributor

@rimiti I'll let you merge and release. We'll discuss a path forward on slack.

@rimiti
Copy link
Contributor Author

rimiti commented Mar 15, 2019

it should have been tagged as v4.0.0-alpha.1 but we can rewrite the past.

@rimiti rimiti merged commit 078c835 into master Mar 15, 2019
@rimiti rimiti deleted the hotfix branch March 15, 2019 12:44
@Floby
Copy link

Floby commented Mar 15, 2019

Thank all of you. At least this was published on a major/breaking version anyway. Could be worse. :)

@bajtos
Copy link
Contributor

bajtos commented Mar 15, 2019

FWIW, I believe the actual root cause of the problem is in superagent and should be already fixed by my pull request ladjs/superagent#1468. Once a new version of superagent is released, we should be good to use superagent@4 in supertest again.

@artparks
Copy link

artparks commented Sep 3, 2019

any news on upgrading this?

@mdhornet90
Copy link

Checked out supertest on my machine and updated its dependency on superagent first to 4.0.0, and confirmed that there was a failing test due to a timeout being exceeded (should assert using promises). I then changed the dependency version to 5.1.0 (which includes @bajtos fix) and confirmed that all tests pass. Can this move forward or does there need to be more verification?

@rimiti
Copy link
Contributor Author

rimiti commented Oct 2, 2019

I'll try to pre-release a version today.
Thanks @bajtos and @mdhornet90

@rimiti rimiti mentioned this pull request Oct 2, 2019
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 this pull request may close these issues.

None yet

7 participants