Hi Bertrand, I took your comments and adapted the two modules, as well as changed the samples tests module to use the new clients and rules.
Clients: https://github.com/dulvac/sling/tree/sling-testing-tools/testing/http/clients * Removed html and microdata part * s/Quickstart/Instance/ Rules: https://github.com/dulvac/sling/tree/sling-testing-tools/testing/junit/rules * Moved to junit/rules * Added SlingInstanceRule that for now delegates the conditional startup logic of a sling instance to SlingTestBase Samples: https://github.com/dulvac/sling/tree/sling-testing-tools/testing/samples/bundle-with-it/src/test/java/org/apache/sling/testing/samples/bundlewit * Adapted the existing tests * Added a FilterIT that showcases the possibility to filter tests at runtime + also see the failsafe section of the pom file Hope this touches on all the points. Anything else that stands out? - Andrei On Thu, Apr 28, 2016 at 12:26 PM Andrei Dulvac <andrei.dul...@gmail.com> wrote: > Hi Bertrand, > > On Thu, Apr 28, 2016 at 11:54 AM Bertrand Delacretaz < > bdelacre...@apache.org> wrote: > >> Hi Andrei, >> >> On Tue, Apr 26, 2016 at 3:53 PM, Andrei Dulvac <andrei.dul...@gmail.com> >> 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 >> >
