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 with-context macro for conviently wrapping collapsers in thier own context #135

Merged
merged 2 commits into from
Apr 4, 2013

Conversation

dinedal
Copy link
Contributor

@dinedal dinedal commented Apr 2, 2013

Closes #132

@cloudbees-pull-request-builder

Hystrix-pull-requests #7 FAILURE
Looks like there's a problem with this pull request

@dinedal
Copy link
Contributor Author

dinedal commented Apr 2, 2013

Is there any easy way to run tests locally for the clojure bindings? without a project.clj, there's no way to invoke lein test

@cloudbees-pull-request-builder

Hystrix-pull-requests #8 FAILURE
Looks like there's a problem with this pull request

@dinedal
Copy link
Contributor Author

dinedal commented Apr 2, 2013

I can't recreate this issue locally, even after putting in the effort to get tests working at all.

@daveray I would appreciate some help here, if possible

@daveray
Copy link
Contributor

daveray commented Apr 2, 2013

@dinedal you're code looks fine. Maybe @benjchristensen can shed some light on this test failure. It doesn't look related to the Clojure code though.

You can run the tests with ./gradlew test.

@daveray
Copy link
Contributor

daveray commented Apr 2, 2013

... and for what it's worth, you can get an nrepl server to connect to (from your editor, lein repl, etc) with gradlew nrepl. I usually just have vim connected to that and run the tests from vim.

@daveray
Copy link
Contributor

daveray commented Apr 2, 2013

@dinedal looks like there's now a failure in the tests with your changes. There's no need to wrap (to-upper-collapser) in a context because the tests themselves are already wrapped with a context management fixture.

@dinedal
Copy link
Contributor Author

dinedal commented Apr 2, 2013

@daveray Thanks, I can revert and push it again

@cloudbees-pull-request-builder

Hystrix-pull-requests #9 FAILURE
Looks like there's a problem with this pull request

@dinedal
Copy link
Contributor Author

dinedal commented Apr 2, 2013

@daveray @benjchristensen

Thanks for the info on how to run the tests! That worked, but locally I see 100% success now. Any pointers would be helpful, would love to contribute to this project.

100% success

@daveray
Copy link
Contributor

daveray commented Apr 3, 2013

@dinedal your new changes work fine for me. I wonder if it should be with-request-context just to be explicit?

I'll merge them in once I get confirmation from @benjchristensen that the cloudbees errors are unrelated. I seem to recall there was a concurrency related test that worked some places, but failed others.

@dinedal
Copy link
Contributor Author

dinedal commented Apr 3, 2013

WRT the name, I can make that change tomorrow no problem.

On Apr 2, 2013, at 9:08 PM, Dave Ray [email protected] wrote:

@dinedal https://github.com/dinedal your new changes work fine for me. I
wonder if it should be with-request-context just to be explicit?

I'll merge them in once I get confirmation from
@benjchristensenhttps://github.com/benjchristensenthat the cloudbees
errors are unrelated. I seem to recall there was a
concurrency related test that worked some places, but failed others.


Reply to this email directly or view it on
GitHubhttps://github.com//pull/135#issuecomment-15816751
.

@cloudbees-pull-request-builder

Hystrix-pull-requests #10 FAILURE
Looks like there's a problem with this pull request

@benjchristensen
Copy link
Contributor

Ignore the CloudBees issues ... I have some non-deterministic tests that work on our internal build system but not CloudBees ... one of these days I'll get those fixed.

@daveray has confirmed this is good so I'll merge.

benjchristensen added a commit that referenced this pull request Apr 4, 2013
Add `with-context` macro for conviently wrapping collapsers in thier own context
@benjchristensen benjchristensen merged commit a402705 into Netflix:master Apr 4, 2013
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.

Hystrix-clj IllegalStateException when using collapsers
4 participants