spring-cloud / spring-cloud-netflix

Integration with Netflix OSS components
http://cloud.spring.io/spring-cloud-netflix/
Apache License 2.0
4.86k stars 2.44k forks source link

Unable to build or run all tests successfully on windows #2086

Closed msamad closed 7 years ago

msamad commented 7 years ago

Hi, I cloned master and tried mvnw install but have not been able to get it running successfully - complains about failed tests in 'Spring Cloud Netflix core'. Have also tried running all tests from within Spring IDE, still the same tests fail.

org.springframework.cloud.netflix.feign.support.SpringMvcContractTests.txt org.springframework.cloud.netflix.feign.valid.FeignOkHttpTests.txt org.springframework.cloud.netflix.metrics.MetricsRestTemplateTests.txt org.springframework.cloud.netflix.resttemplate.RestTemplateRetryTests.txt org.springframework.cloud.netflix.ribbon.okhttp.SpringRetryDisableOkHttpClientTests.txt org.springframework.cloud.netflix.ribbon.okhttp.SpringRetryEnabledOkHttpClientTests.txt org.springframework.cloud.netflix.ribbon.RibbonClientConfigurationTests.txt org.springframework.cloud.netflix.ribbon.RibbonClientHttpRequestFactoryTests.txt org.springframework.cloud.netflix.ribbon.SpringClientFactoryTests.txt org.springframework.cloud.netflix.zuul.filters.route.okhttp.OkHttpRibbonRetryIntegrationTests.txt org.springframework.cloud.netflix.zuul.filters.route.restclient.RestClientRibbonCommandIntegrationTests.txt

Are there any services that I need to run for the above tests to pass? Can't find any documentation about that, can someone kindly help and point me in the right direction?

I am not on Linux - using Windows 10, if that makes any difference.

Thank you

spencergibb commented 7 years ago

I am unsure. We require java 8 to compile. Tests pass on jenkins, circleci and our developer machinces. I don't have access to a windows box to try.

Why are you building?

msamad commented 7 years ago

Thanks, I am using java 8 and did look into the circleci file but couldn't find anything different. Seemed a bit strange that build was passing there but not on my machine that is why I asked the question in case I was making a very obvious mistake. I'll try and do it on Linux machine.

Was looking into Zuul and was hoping to contribute to it - to add few functionalities. Just wanted to make sure I could build and run all tests before I start playing around.

spencergibb commented 7 years ago

Maybe someone else on the team has access to a Windows box that can try? @ryanjbaxter, you're the only one I can think that might.

msamad commented 7 years ago

So tried it with docker image maven:3.5.0-jdk-8 and was able to build it successfully. Not sure what causes it to fail on Windows 10.

I'll just use the docker image to build and test.

Cheers!

ryanjbaxter commented 7 years ago

Sorry I don't :(

msamad commented 7 years ago

Hmmm ok, guess I'll have to try and fix the tests that are failing - the code should build on Windows as well since it is all Java. I'll need help from you guys as you might be able to spot something right away while I may have to spend hours digging as I am just starting with the project.

The first one that I looked into is OkHttpRibbonRetryIntegrationTests which is extending RibbonRetryIntegrationTestBase.

Here is the image, you can see the stacktrace in my first comment as well. okhttpribbonretryintegrationtests

My Analysis: HttpClientRibbonRetryIntegrationTests is also extending RibbonRetryIntegrationTestBase but all of its tests are passing.

In RibbonCommandFactoryConfiguration if I comment out the following

        @Bean
        @ConditionalOnMissingBean
        public RibbonCommandFactory<?> ribbonCommandFactory(
                SpringClientFactory clientFactory, ZuulProperties zuulProperties) {
            return new OkHttpRibbonCommandFactory(clientFactory, zuulProperties,
                    zuulFallbackProviders);
        }

then OkHttpRibbonRetryIntegrationTests passes all of its tests and HttpClientRibbonRetryIntegrationTests passes as well.

My conclusion is that OkHttpRibbonRetryIntegrationTests should not be picking up the above Bean RibbonCommandFactory<?> but for some reason it is picking that one up or something else is not loading as expected.

Your thoughts? @spencergibb @ryanjbaxter

Thanks

ryanjbaxter commented 7 years ago

Are they all failing with a 404? Are there any stacktraces from the application itself?

msamad commented 7 years ago

org.springframework.cloud.netflix.zuul.filters.route.okhttp.OkHttpRibbonRetryIntegrationTests.txt The other two are failing with 200.

disableRetry is failing with

disableRetry(org.springframework.cloud.netflix.zuul.filters.route.okhttp.OkHttpRibbonRetryIntegrationTests)  Time elapsed: 1.139 sec  <<< FAILURE!
java.lang.AssertionError: expected:<500> but was:<200>

globalRetryDisabled is failing with

globalRetryDisabled(org.springframework.cloud.netflix.zuul.filters.route.okhttp.OkHttpRibbonRetryIntegrationTests)  Time elapsed: 1.116 sec  <<< FAILURE!
java.lang.AssertionError: expected:<500> but was:<200>

The application runs fine - can't see any stacktrace there.

ryanjbaxter commented 7 years ago

You don't happen to have a config server serving configuration running in your environment do you?

msamad commented 7 years ago

No, don't have anything else running. Just executing the simple JUnit test for OkHttpRibbonRetryIntegrationTests class.

ryanjbaxter commented 7 years ago

Besides debugging the code I cant think of any other reason why this would happen...

msamad commented 7 years ago

ok, I'll debug it and see if I can spot anything. @ryanjbaxter Can you please confirm if the following bean should be loaded when running these tests in OkHttpRibbonRetryIntegrationTests or not. What happens when you run the test class on your machine?

In RibbonCommandFactoryConfiguration

        @Bean
        @ConditionalOnMissingBean
        public RibbonCommandFactory<?> ribbonCommandFactory(
                SpringClientFactory clientFactory, ZuulProperties zuulProperties) {
            return new OkHttpRibbonCommandFactory(clientFactory, zuulProperties,
                    zuulFallbackProviders);
        }

That will be really helpful if I can eliminate this.

Thank you

ryanjbaxter commented 7 years ago

Yes that bean is created when I run that test

msamad commented 7 years ago

Ahaa! Found the tipping point. The issue is in RibbonClientConfiguration. Not the class itself, but the way configuration classes in it are being loaded and revolves around @ConditionalOnProperty annotation.

When I run tests in OkHttpRibbonRetryIntegrationTests, it is suppose to load the following configuration class

    @Configuration
    @ConditionalOnProperty("ribbon.okhttp.enabled")
    @ConditionalOnClass(name = "okhttp3.OkHttpClient")
    protected static class OkHttpRibbonConfiguration {

but it loads the following

    @Configuration
    @ConditionalOnProperty(name = "ribbon.httpclient.enabled", matchIfMissing = true)
    protected static class HttpClientRibbonConfiguration {

Now here is the weird part - it seems that the classes are being loaded in alphabetical order and @ConditionalOnProperty is being honored aggressively i.e. if the first encountered condition satisfies, (matchIfMissing = true in the above case) then that configuration class is loaded.

Just did a hack and prefixed the OkHttpRibbonConfiguration with character A and it started loading correctly. Did the same with RestClientRibbonConfiguration and all of tests except 2 started passing. So now they look like - notice the prefix A

    @Configuration
    @ConditionalOnProperty("ribbon.okhttp.enabled")
    @ConditionalOnClass(name = "okhttp3.OkHttpClient")
    protected static class AOkHttpRibbonConfiguration {
    @Configuration
    @RibbonAutoConfiguration.ConditionalOnRibbonRestClient
    protected static class ARestClientRibbonConfiguration {

One of the remaining failing tests was FeignOkHttpTests.testFeignClientType, did the same hack with OkHttpFeignLoadBalancedConfiguration and it passed.

SpringMvcContractTests.testProcessAnnotations_ListParamsWithoutName is now the only one failing, this one is a bit different - will look into it to see what's wrong with it.

Weird, right? So seems like there is something fundamentally wrong here, I'll try and see what exactly it is. The hack is just to prove that it works if alphabetical order is correct but that should not matter and configurations should be loaded properly based on annotations and not some unknown (at this point) class loading order.

My guess is that one of the below three might be the culprit which behaves differently on Linux and Windows

Your views @ryanjbaxter @spencergibb ?

spencergibb commented 7 years ago

Doesn't make much sense to me. @wilkinsona any thoughts?

wilkinsona commented 7 years ago

It looks like RibbonClientConfiguration is assuming that its inner classes will be considered in the order in which they are declared in the source code. That's not a safe assumption to make. It's entirely dependant on the byte code produced by the compiler and how ASM loads that byte code. You either need to make the property conditions mutually exclusive, or move the configurations to top-level classes and use @Import which does provide ordering guarantees.

spencergibb commented 7 years ago

Thanks! @wilkinsona

spencergibb commented 7 years ago

Closed via 6b530b026966faad7e88efd0c67a68e93195bfcd

spencergibb commented 7 years ago

Merged to master and 2.0.x

spencergibb commented 7 years ago

@msamad would you be willing to try out a snapshot when it is available (shortly)?

msamad commented 7 years ago

Thank you @spencergibb Looks like you missed FeignRibbonClientAutoConfiguration, it has the same issue. The rest are fine now, passing their tests.

I still haven't figured out what is wrong with SpringMvcContractTests.testProcessAnnotations_ListParamsWithoutName, will debug it more to see if I can find anything. This will be the only remaining failing test once you fix FeignRibbonClientAutoConfiguration. Should we keep this issue opened here or do you want me to raise a new issue for SpringMvcContractTests.testProcessAnnotations_ListParamsWithoutName?

Thanks again. :)

spencergibb commented 7 years ago

No

spencergibb commented 7 years ago

Closed via 97f1655ff7b0078b0c92f37042d3617c560a0668

spencergibb commented 7 years ago

Ping @msamad to try again :-)

msamad commented 7 years ago

Yup, all tests passing now apart from pringMvcContractTests.testProcessAnnotations_ListParamsWithoutName - I'll see what I can find for this one.

Thank you @spencergibb :)

msamad commented 7 years ago

Hi, Figured out what was wrong with SpringMvcContractTests.testProcessAnnotations_ListParamsWithoutName. It was throwing this error RequestParam.value() was empty on parameter 0.

So selected Store information about method parameters (usable via reflection) in eclipse and that fixed it.

image

Finally, all tests are passing... phew!

Thank you

WennaDu commented 5 years ago

I want to rewrite ribbon.ConnectTimeout( from mysql datebase ),so I wrote class ConsumerRibbonClientConfiguration extend RibbonClientConfiguration and Override the method ribbonClientConfig. But when debug, the class ConsumerRibbonClientConfiguration not execute ? how to do ?