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

Lack of test suites no longer breaks builds #347

Merged
merged 2 commits into from
Apr 25, 2017
Merged

Lack of test suites no longer breaks builds #347

merged 2 commits into from
Apr 25, 2017

Conversation

andrehp
Copy link
Contributor

@andrehp andrehp commented Apr 25, 2017

No description provided.

@onsi
Copy link
Owner

onsi commented Apr 25, 2017

Looks like the build broke:

/home/travis/gopath/src/github.com/onsi/ginkgo/integration/run_test.go:161

should be a quick fix if you have a moment to update the PR. (sorry to be asking for such a basic PR -I'm away from my computer and making these changes on my iPad would be error-prone).

@benmoss
Copy link
Contributor

benmoss commented Apr 25, 2017

you can just get rid of this assertion or change it to a 0:

Eventually(session).Should(gexec.Exit(1))

@andrehp
Copy link
Contributor Author

andrehp commented Apr 25, 2017

Yeah, sorry, I didn't notice there was a test as well. Fixed the PR

@onsi onsi merged commit 83438ca into onsi:master Apr 25, 2017
@cehbz
Copy link
Contributor

cehbz commented Jun 29, 2017

How would you feel about putting this behavior under a command line option? I've lost a lot of time to this so I've already added it to a fork.

https://github.com/charles-haynes/ginkgo/commit/2202d205c5e87c307359c69b45105ed9e85cdc6a

@onsi
Copy link
Owner

onsi commented Jun 29, 2017

sure. please shoot over a PR @charles-haynes - the commit in your fork LGTM.

Can you also update the docs in the gh-pages branch to mention the flag?

@cehbz
Copy link
Contributor

cehbz commented Jun 30, 2017

I've created a PR for the code, am doing the gh-pages now.

@benmoss
Copy link
Contributor

benmoss commented Jun 30, 2017

Sorry about that @charles-haynes, I never envisioned this would cause so many problems :(

@cehbz
Copy link
Contributor

cehbz commented Jul 3, 2017

no no @benmoss! Your work was super helpful. It was only when yet another new starter on our project had been bitten by it that I decided to go digging for a solution and what do you know - you had already done all the work. All I had to do was revive it and put it on a flag - voila! Thanks.

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.

4 participants