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

Add server response log when status code error #706

Closed
wants to merge 1 commit into from

Conversation

WarleyGabriel
Copy link

@WarleyGabriel WarleyGabriel commented Feb 1, 2021

This pull request solve this issue: #631

@niftylettuce
Copy link
Collaborator

I think this would be a pretty big breaking change, hmm

@coveralls
Copy link

coveralls commented Feb 1, 2021

Pull Request Test Coverage Report for Build 495

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.447%

Totals Coverage Status
Change from base Build 493: 0.0%
Covered Lines: 146
Relevant Lines: 150

💛 - Coveralls

@danilofuchs
Copy link

Why would this be a breaking change? It makes the assertions more useful for debugging, but does not affect the assertion itself. A test that does not pass will continue not passing, but now with more info

@niftylettuce
Copy link
Collaborator

I think this is a breaking change because one would normally expect the error messages to be much shorter.

I also think that stringifying the body in the message is probably bad practice, as the body could be a huge response payload (e.g. HTML by accident instead of JSON). I think perhaps declaring the error, e.g. const err = new Error(...) and then making a private property on it, such as err._res = res would be more useful.

@WarleyGabriel
Copy link
Author

Since this is not gonna be merged, closing it.

@titanism
Copy link
Collaborator

This should be a new PR, and as suggested, there should be another property on the error object.

We suggest to add both err.response and err.response_stringified.

Also this will be a breaking change, since some may have tests written that explicitly check the err object for certain values and properties.

@titanism
Copy link
Collaborator

We would merge this if you submitted a new PR with suggestions above @WarleyGabriel. Thank you 🙏

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

5 participants