Hi Bertrand, On Thu, Apr 28, 2016 at 11:54 AM Bertrand Delacretaz <[email protected]> wrote:
> Hi Andrei, > > On Tue, Apr 26, 2016 at 3:53 PM, Andrei Dulvac <[email protected]> > wrote: > > ...The sling-testing-tools [0] has become a container for different small > > pieces of code that don't fit in the other projects under testing/ [1]... > > I agree that that module is not consistent, it contains unrelated > classes as you mention. > > > The changes would be the following: > > * leave the tools/ (sling testing tools) untouched, with the idea to > > eventually deprecate it if the other modules fit the needs... > > Sounds good, but we'd have to be careful to actually do this and > deprecate that module as soon as we're happy with the alternatives > that you suggest. > > In general I like the next modules that you suggest, but IMO any such > patches should come with additional patches that demonstrate how this > new stuff can be used in existing or new tests in our core codebase, > and how it makes our tests nicer. > Sure, I can provide some sample tests using them. Basically it's nothing super fancy, just a coherent design across http "clients" > > > * create a new module, called serversetup with the jarexec and > serversetup > > packages copied from sling-testing-tools. > > sounds good > > > * create module http/clients with some stuff like the WebconsoleClient > > copied from tools/ and a bunch of classes that replace the > RequestExecutor > > and client with Threadsafe, well-tested and HttpClient-compatible http > > tooling... > > sounds good as well in general, but I'm not sure if everything belongs > there in your branch. AbstractPoller for example looks independent > from HTTP clients. > The AbstractPoller is the equivalent of RetryLoop, which is in sling-testing-tools next to the clients. It's also used in some methods by the clients, to allow retrying an operation. > > The clients/html stuff also looks relatively independent. > It's using the SlingClient to allow a hypermedia-driven approach to these sling operations, rather than using http verbs. But I see the point and we can remove it. > > There's clients/quickstart, but we don't have a quickstart in Sling, > should this have a better name? Does it really belong in there? > It's used to allow configuring clients based on config from the outside (alongside programatic configuration) based on some system properties. We've talked offline about this once and how it can evolve into a bigger set of available constraints regarding the "available instances". You're right about the "quickstart" name. I can do a << s/Quickstart/Instance/g >> clients/html/microdata also looks fairly specific. > It's related to the html client, we can also remove it. > > All in all it looks like there's a lot in that http/clients module, > I'm wondering if it all makes sense for the Sling core (as /testing is > part of that) or if some of that is too specific, and also if that > should be broken up into smaller, more specific modules. Some of which > might to under /contrib instead. > I think those classes work together to provide an already minimal toolset to perform sling operations, as well as building on top of it. With the mentions above, where the html + microdata stuff can go out. > > >...create module http/rules with junit tooling dependent on the clients > > which follow the more modern approach to junit rules rather than test > base > > classes and provide ootb rules with clients, junit filtering, configuring > > references to available sling instances,... > > There's lots of cool stuff in there but most of it doesn't look > specifically related to HTTP. Should that module also be broken up in > smaller pieces? Or can it be made more minimalistic by removing some > rules that are specific to your way of working but might not be > relevant for Sling as a whole and could move to /contrib instead? > > Don't get me wrong, I like your idea of cleaning up testing tools very > much. I'm just wary of introducing new somewhat big and catch-all > modules, I'd prefer whatever goes under /testing to be as lean as > possible, some non-core classes and rules might move to contrib to > reduce the size of what needs to be maintained at core level. > Thanks for the vote of confidence (again) and for sharing my interest to do some maintenance on the sling-testing-tools. Regarding the rules module, they have a couple of generic rules and SlingBaseRule - that is supposed to replace the SlingTestBase with the more modern junit approach. Those rules are tied to the http client module. e.g. TestDescriptionRule which allows setting some info regarding the current junit test being run (class + method) that is then being picked up by the http clients if available, from a static context, and sets some headers in the http call that sling logs in the backend. There is not much there either, to be honest: * Rule to filter some junit tests at runtime based on some conditions * TestDescriptionRule mentioned above * Rule for timeout * Rule for allowing a sticky session cookie in the htp clients be used across different calls in the same test * Rule for InstanceConfig, which is a mechanism that allows saving config - mostly though http and restoring it at the end of the test I think splitting those into multiple modules would basically break them. The only split I could do is separating the clients form the junit rules, which is already a big improvement from where sling-testing-tools are now. Regarding the different way of working, I think these apply 100% to sling tests, both the clients and rules. And looking at the usage of sling-testing-tools, the current way of working is using the RequestExecutor to make http calls. I haven't seen anything else. I will make another branch with the tweaks i mentioned above for the clients. I don't really know how to proceed regarding your other concerns... Thanks for looking at it! - Andrei > -Bertrand >
