On Sun, Feb 15, 2015 at 6:03 PM, sebb <[email protected]> wrote: > On 15 February 2015 at 16:53, Philippe Mouawad > <[email protected]> wrote: > > On Sunday, February 15, 2015, sebb <[email protected]> wrote: > > > >> On 15 February 2015 at 16:21, Philippe Mouawad > >> <[email protected] <javascript:;>> wrote: > >> > On Sunday, February 15, 2015, sebb <[email protected] <javascript:;>> > >> wrote: > >> > > >> >> On 14 February 2015 at 17:53, Philippe Mouawad > >> >> <[email protected] <javascript:;> <javascript:;>> wrote: > >> >> > Hello, > >> >> > Looking at code of class, I see that in fact HTTPSamplerProxy > which is > >> >> used > >> >> > by Http Sampler is not tested , instead it appears the class tests > : > >> >> > - HTTPSampler3 (which is not used anywhere anymore, I propose to > >> delete > >> >> it, > >> >> > I will create a bugzilla for this ) > >> >> > >> >> HTTPSampler3 is currently used by test code. > >> >> > >> >> Why not just use > >> HTTPSamplerProxy(HTTPSamplerFactory.IMPL_HTTP_CLIENT4); > >> > instead ? > >> > >> You said it was not used. But it is. > > > > I meant in non test code but ok :) > > > > > >> > >> Replacing it with a call to HTTPSamplerProxy is a separate issue. > >> > >> >> - HTTPSampler2 (used by SoapSampler) > >> >> > - HTTPSampler > >> >> > > >> >> > > >> >> > I suggest we replace code by : > >> >> > private HTTPSamplerBase createHttpSampler(int samplerType) { > >> >> > switch(samplerType) { > >> >> > case HTTP_SAMPLER: > >> >> > return new > >> >> > HTTPSamplerProxy(HTTPSamplerFactory.HTTP_SAMPLER_JAVA); > >> >> > case HTTP_SAMPLER2: > >> >> > return new > >> >> > HTTPSamplerProxy(HTTPSamplerFactory.IMPL_HTTP_CLIENT3_1); > >> >> > case HTTP_SAMPLER3: > >> >> > return new > >> >> > HTTPSamplerProxy(HTTPSamplerFactory.IMPL_HTTP_CLIENT4); > >> >> > default: > >> >> > break; > >> >> > } > >> >> > throw new IllegalArgumentException("Unexpected type: > >> >> "+samplerType); > >> >> > } > >> >> > > >> >> > If everybody is OK , I will create a bugzilla for this. > >> >> > >> >> That assumes HTTPSamplerProxy is working OK. > >> > > >> > I don't understand what you mean here. > >> > Shouldn't this class be the one under kunit tests instead of ones that > >> are > >> > rarely or not used. > >> > >> Probably, but again that is a slightly different issue. > > > > > > I don't share your view. My purpose here is to point that some junit code > > is not doing tests on classes that are part of jmeter core and part of it > > are doing it on deprecated code. > > > > Fine, but that is a separate issue. > > > > >> > >> >> So I don't think it is a good replacement without further tests to > >> >> check HTTPSamplerProxy actually does what one expects/ > >> > > >> > > >> > I don't understand sebb. > >> > >> There are no tests to ensure that HTTPSamplerProxy actually generates > >> the correct sampler. > > > > > > You mean in terms of implementation returned ? > > Yes. > > > > >> It could always return the JAVA sampler and AFAICT the existing tests > >> would not catch that. > > > > > > Ok we could improve this, at least doing the change I propose would > improve > > things by applying tests on most used jmeter class. Overall as long as it > > improves things why are you against that change . > > I never said I was totally against the change. > > I said that I was against it _as it stands_, because it makes > assumptions that are not tested. > > Changing the test as originally proposed makes the existing test worse > UNLESS the assumptions underlying the change are also tested. > I don't believe so, but ok. Regarding this test unless we expose impl this is not doable cleanly.
> > > > > > >> There need to be tests that ensure HTTPSamplerProxy is working as > expected. > > > > as its logic is rather simple it could easily be added. It's true that I > > suppose it works. > > So that assumption needs to be tested. > > > > >> Otherwise tests that depend on it working correctly are not very useful. > >> > >> >> > >> >> > I also suggest we replace new HttpSampler() by : > >> >> > return new HTTPSamplerProxy(HTTPSamplerFactory.HTTP_SAMPLER_JAVA); > >> >> > >> >> Where is this to be replaced? > >> > > > > > > -- > > Cordialement. > > Philippe Mouawad. > -- Cordialement. Philippe Mouawad.
