Add a When() synonym for Context()

[akshaymankar]

Solves #348

[williammartin]

Thanks for the PR @akshaymankar I'm gonna tag this as needs-review because there's a bunch of changes in here, but definitely on board with the premise!

[iandelahorne]

Would you be open to rebasing off master and repushing so we can get coverage on 1.9 now that our Travis configuration runs against it? I ran your branch locally on 1.9.1 and it failed, but if I rebased off master tests passed.

[iandelahorne]

I like this functionality! In my mind, When is a special case of Context and we should treat it as such in our tests. I don't feel that it warrants changing all the existing Contexts that are in the tests. Are you open to adding tests to add coverage for When, but backing out the Context("When... -> When( changes?

[adamcohen]

Good to see this PR getting some traction. I've been looking forward to seeing it get merged since I use When quite frequently in my tests.

[akshaymankar]

@lflux Thanks for the review. I will rebase the branch in my flex-hour or over the weekend.

I couldn't find a place to add tests for dsl code. If you could point me to it, I'll add tests for the functions related to When.

I am not sure why should Context("When.. toWhen(... be backed out, could you please clarify?

[williammartin]

I don't really want this merged because it opens the door to DSL bloat for all the conjunctions e.g. And(, But(, Because(, AndWhen( (cause nesting When( in When( is weird), etc. Less problematic, it also means that during a refactoring stage you could pretty easily change textual description but forget to change correlated method call, leaving confusing test output.

Pragmatically though, I understand that people are interested in this and there is some precedent for It( and Specify( in e986a52 so it's probably fine to do so but I'd like a few things changed.

Seems like instead of doing:

globalSuite.PushContainerNode("when "+text, body, types.FlagTypeFocused, codelocation.New(1))
return true

we could do:

return Context(.....

In the same way Specify( does, just to DRY this out a little.

Due to my admittedly flimsy reasons at the top, I'd also prefer not to see all the Contexts changed to Whens, and rather have it a special case.

Finally, definitely want some docs to be PRed along with this to the gh-pages branch.

TL;DR:

Changes I'd like:

1. DRY When( by following the Specify( example
2. Test When( as a special case
3. PR docs.

But I'm happy to chat about any/all of these!

Thanks.

[akshaymankar]

I am not so sure about the DSL bloat problem. Right now it is not such a big problem, but if it does start bloating maybe that can be a separate project.

My views/comments on the requested changes:

1. DRY When( by following the Specify( example

I already have a problem with the Specify works. Whenever I have a failure, ginkgo tells me the failure is in ginkgo's code where Specify is defined. As somebody who has seen his tests red, I wouldn't want ginkgo's line numbers in my report.

2. Test When( as a special case

I agree with the arguments given. I will revert changes to the tests and add another test.

3. PR docs

I will write it up and send another PR.

[williammartin]

@akshaymankar I didn't realise Specify( printed a stack trace like that but I'm not able to reproduce that:

Specify("doesn't print the right stack trace", func() {
	Fail("fail the test")
})

->

Users/pivotal/go/src/github.com/williammartin/ginkgoleaktest/ginkgoleaktest_test.go:8
  doesn't print the right stack trace [It]
  /Users/pivotal/go/src/github.com/onsi/ginkgo/ginkgo_dsl.go:394

  fail the test

  /Users/pivotal/go/src/github.com/williammartin/ginkgoleaktest/ginkgoleaktest_test.go:11

Totally agree with you if that is what happens though. If you have a reproduction, we can open a separate issue to fix it. Thanks.

[akshaymankar]

@williammartin Third line in the output that you posted:

/Users/pivotal/go/src/github.com/onsi/ginkgo/ginkgo_dsl.go:394

It usually prints the line where It is called, but for specify it is always ginkgo_dsl.go:394

[williammartin]

Doh! Thanks for keeping me right! I created #414 and PR #415 if you want to continue that discussion elsewhere. Otherwise I'm good with this if we fix up the special case When test as talked about above. Thanks.

[williammartin]

Didn't see that you'd updated this PR, sorry! Happy to merge now.

Two outstanding things we can follow up on after is updating the docs, and writing a test, maybe within the progress_fixture that makes use of a When block. Also some comments in internal/suite/suite.go and README.md about nestable DSL, but let's not block this any more. Hopefully @alamages and I can get to this tomorrow evening.

Thanks