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. > > >> 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.
