Skip to content

allow body for HTTPBodyContains and HTTPBodyNotContains for POST #938

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 3, 2020
Merged

allow body for HTTPBodyContains and HTTPBodyNotContains for POST #938

merged 6 commits into from
May 3, 2020

Conversation

gohargasparyan
Copy link
Contributor

Summary

Issue #925
Http Assertions does not allow the creation of a body

Changes

  • Added body argument in HTTPBodyContains and HTTPBodyNotContains
  • in tests added httpPostHandler which reads request body and writes to response
  • In HTTPBody checking if there are url params , otherwise not adding to url

Related issues

#925

@gohargasparyan gohargasparyan mentioned this pull request Apr 29, 2020
Closed
@gohargasparyan gohargasparyan changed the title allow body for HTTPBodyContains and HTTPBodyNotContains for POST WIP: allow body for HTTPBodyContains and HTTPBodyNotContains for POST Apr 29, 2020
@gohargasparyan gohargasparyan changed the title WIP: allow body for HTTPBodyContains and HTTPBodyNotContains for POST allow body for HTTPBodyContains and HTTPBodyNotContains for POST Apr 29, 2020
boyan-soubachov
boyan-soubachov requested changes May 2, 2020
View reviewed changes
Copy link
Collaborator

@boyan-soubachov boyan-soubachov 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 for your effort, this looks great! :)

I have left a small comment about testing the new logic added. Beyond that, I'd be happy to merge this once it's resolved.

assert/http_assertions.go
Comment on lines +118 to +121
if values != nil {
url = url + "?" + values.Encode()
}
req, err := http.NewRequest(method, url, body)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that this logic has been added, would it not make sense to add new test cases to check for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will add some tests

boyan-soubachov
boyan-soubachov approved these changes May 3, 2020
View reviewed changes
Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you!

@boyan-soubachov boyan-soubachov merged commit 136026f into stretchr:master May 3, 2020
@boyan-soubachov boyan-soubachov added this to the v1.6.0 milestone May 3, 2020
@dpajkovic
Copy link

Would it be possible to give example for the simple GET request in GoDoc/pkg.go.dev documentation? The example given

assert.HTTPBodyContains(t, myHandler, "GET", "www.google.com", nil, "I'm Feeling Lucky")

no longer works, and more frustratingly none of the test scripts I've written work without rolling back the package version to 1.5.1.

@boyan-soubachov
Copy link
Collaborator

@dpajkovic , apologies. I just realised that I should have tagged this for v2 rather than 1.6.0 😞 .

I will revert this change and release a 1.6.1 quickly.

boyan-soubachov added a commit to boyan-soubachov/testify that referenced this pull request Jun 3, 2020
@boyan-soubachov
Revert "allow body for HTTPBodyContains and HTTPBodyNotContains for
0a986b1 
POST" PR stretchr#938

This reverts commit 136026f.
@boyan-soubachov boyan-soubachov mentioned this pull request Jun 3, 2020
Merged
@gohargasparyan
Copy link
Contributor Author

oh, sorry @dpajkovic missed that part, so what happens to this changes now @boyan-soubachov ?

boyan-soubachov added a commit that referenced this pull request Jun 5, 2020
@boyan-soubachov
Revert "allow body for HTTPBodyContains and HTTPBodyNotContains for
303198d 
POST" PR #938

This reverts commit 136026f.
@boyan-soubachov
Copy link
Collaborator

boyan-soubachov commented Jul 17, 2020 •
edited
Loading

@gohargasparyan, I guess the following options are available to us:
a) Do this work again but only merge it in v2 as it's a breaking change
b) Implement this as a new function/feature
c) Try and make the change backwards-compatible

IMO probably the best approaches would be (in order of preference): c -> a -> b :)

dolmen
dolmen reviewed Jul 29, 2023
View reviewed changes
Copy link
Collaborator

@dolmen dolmen left a comment

Choose a reason for hiding this comment

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

This can't be merged in v1.

And v2 is very far away.

assert/assertion_format.go
@@ -6,6 +6,7 @@
package assert

import (
io "io"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the alias

assert/assertion_forward.go
@@ -6,6 +6,7 @@
package assert

import (
io "io"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the alias

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dolmen dolmen dolmen left review comments

@boyan-soubachov boyan-soubachov boyan-soubachov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@gohargasparyan @dpajkovic @boyan-soubachov @dolmen