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

ghttp.Server doesn't work with gomega.NewGomegaWithT(t) #321

Closed
s12chung opened this issue Dec 17, 2018 · 9 comments · Fixed by #376
Closed

ghttp.Server doesn't work with gomega.NewGomegaWithT(t) #321

s12chung opened this issue Dec 17, 2018 · 9 comments · Fixed by #376
Labels

Comments

@s12chung
Copy link

Please see https://gist.github.com/s12chung/8ea6d4919b302beef81d2646161c551a

@williammartin
Copy link
Sponsor Collaborator

If I understand you correctly, you're saying that this block:

func TestXUnitNewGomegaWithT(t *testing.T) {
	g := gomega.NewGomegaWithT(t)

	server := ghttp.NewServer()
	server.AppendHandlers(
		ghttp.CombineHandlers(
			ghttp.VerifyRequest(http.MethodPost, mockPath),
			ghttp.RespondWith(http.StatusOK, mockResponse),
		),
	)

	r, _ := http.Post(server.URL()+mockPath, mime.TypeByExtension(".json"), bytes.NewBuffer([]byte{}))
	bytes, _ := ioutil.ReadAll(r.Body)
	defer r.Body.Close()
	g.Expect(string(bytes)).To(gomega.Equal(mockResponse))
}

Fails, when it should pass?

@s12chung
Copy link
Author

yes.

@williammartin
Copy link
Sponsor Collaborator

williammartin commented Dec 17, 2018

I suppose this sort of makes sense, since the Verify functions in ghttp don't know anything about the NewGomegaWithT. If we comment out the VerifyRequest line, the test begins to pass again.

I'll have a think about potential solutions, also open to ideas :)

@williammartin
Copy link
Sponsor Collaborator

I added a branch (da2dee8) with some thoughts. I didn't give it a huge amount of consideration, just wanted to get some ideas down quickly.

Using this branch we can rewrite your first test:

func TestXUnitNewGomegaWithT(t *testing.T) {
	g := gomega.NewGomegaWithT(t)

	gh := ghttp.NewGHTTPWithGomega(g)
	server := ghttp.NewServerWithGomega(g)
	server.AppendHandlers(
		gh.CombineHandlers(
			gh.VerifyRequest(http.MethodPost, mockPath),
			gh.RespondWith(http.StatusOK, mockResponse),
		),
	)

	r, _ := http.Post(server.URL()+mockPath, mime.TypeByExtension(".json"), bytes.NewBuffer([]byte{}))
	bytes, _ := ioutil.ReadAll(r.Body)
	defer r.Body.Close()

	g.Expect(string(bytes)).To(gomega.Equal(mockResponse))
}

Would be interested to see if:

  1. This functionally works for you?
  2. It reads acceptably?

I haven't written any tests and there's surely some refactoring I could do for code reuse.

@s12chung
Copy link
Author

  1. Functionally, it works :)
  2. It reads alright, I can't think of any better.

thanks for getting back so quick.

@xanderflood
Copy link
Contributor

Is there any plan to upstream the NewServerWithGomega functionality? I'm interested because I'm currently using ghttp in a context where the full ginkgo system conflicts with some other tools I need to use, so I have to use it directly in XUnit tests. I'd like to parallelize my tests, but I don't think I can currently do that without being able to configure each ghttp server to use a separate testing.T.

If there's lingering work to be done to clean up that branch I'd be happy to contribute

@williammartin
Copy link
Sponsor Collaborator

Hi @xanderflood, the main reason this is lingering is because I wasn't very happy with the solution. It was a large amount of duplicated code I was worried about keeping in sync. It's possible I'm missing something obvious, so if you have thoughts on how to reduce the duplication that would definitely be appreciated.

@xanderflood
Copy link
Contributor

@williammartin Needs a little cleanup but how do feel about this general idea? It required making a small addition upstairs in the gomega package so that the package-level Expect function can be passed around as an object that's interchangeable with gomega.WithT

@xanderflood
Copy link
Contributor

@williammartin 🤦‍♀ completely forgot to link to the actual PR #376

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 a pull request may close this issue.

3 participants