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

Proposal: HystrixRequestContext junit rule #1147

Merged
merged 3 commits into from
Apr 19, 2016
Merged

Proposal: HystrixRequestContext junit rule #1147

merged 3 commits into from
Apr 19, 2016

Conversation

caarlos0
Copy link
Contributor

This will make testing commands that requires a HystrixRequestContext
easier in JUnit by using its Rule feature.

An example of usage would be something like:

    @Rule
    public HystrixRequestContextRule request = new HystrixRequestContextRule();

    @Test
    public void blah() {
       // exec a HystrixCommand that requires some kind of caching, for example
    }

I put it in the hystrix-contrib folder, let me know if this is the right thing to do, please.

This will make testing commands that requires a `HystrixRequestContext`
easier in JUnit by using its Rule feature.

An example of usage would be something like:

```java
    @rule
    public HystrixRequestContextRule request = new HystrixRequestContextRule();

    @test
    public void initsContext() {
        MatcherAssert.assertThat(this.request.context(), CoreMatchers.notNullValue());
    }
```

I put it in the `hystrix-contrib` folder, let me know if this is the right thing to do, please.
@cloudbees-pull-request-builder

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

@caarlos0
Copy link
Contributor Author

(build seems unstable)

@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #413 SUCCESS
This pull request looks good

@spencergibb
Copy link
Contributor

👍

@mattrjacobs
Copy link
Contributor

Thanks for the contribution, @caarlos0 . I'm working on increasing the stability of the CI jobs currently.

This seems like a fine change, but I'd like to hold off for a couple of days. I'm trying to figure out how to do things like #1033, where I separate out the lifecycle of hystrix-dashboard from hystrix-core. That potentially has an impact on hystrix-contrib, depending on the details, so I'd like to get that clear in my head before I take on new items in hystrix-contrib.

I will try to have a better, more concrete answer by next week for you. If I don't, please pester me until I do :)

@caarlos0
Copy link
Contributor Author

@mattrjacobs Nice! Will do, thanks!

@mattrjacobs
Copy link
Contributor

@caarlos0 Here's my thought process:

I would like to only include modules in Netflix/Hystrix that Netflix actually uses. In practice, I don't have the bandwidth to do more than bugfixes on anything else. I also become a bottleneck for modules that don't fall into that category. To be clear, this isn't precisely where we're at today, but it's the direction I'd like to head.

With that in mind, this does seem like something that we would use internally (and it's nice and small), so this seems fine to include in hystrix-contrib. Thanks for the contribution (and for the patience)!

@caarlos0
Copy link
Contributor Author

@mattrjacobs Nice, thanks =D

I can help to migrate tests to use it, too =)

@mattrjacobs mattrjacobs merged commit bd2a6ee into Netflix:master Apr 19, 2016
mattrjacobs pushed a commit to mattrjacobs/Hystrix that referenced this pull request Apr 19, 2016
Merge remote-tracking branch 'upstream/pr/1147' into forward-port-pr-1147
@caarlos0 caarlos0 deleted the junit branch April 19, 2016 20:24
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