I just quickly chatted with the author/maintainer of ArchUnit, and a module which depends on every module that should be tested seems to be the best solution. How do you feel about using flink-tests for this vs. having a separate module for this purpose?
Ingo On Mon, Sep 6, 2021 at 3:04 PM Ingo Bürk <i...@ververica.com> wrote: > Hi Chesnay, > > Those are all great questions, and I want to tackle those as well. For the > moment I went per-module, but runtime-wise that isn't ideal the more > modules we'd activate this in. ArchUnit does cache classes between tests, > but if we run them individually per module, we'd still add up quite a bit > of execution time (a single module in my IDE is around 10s with the tests I > currently have implemented, but I suspect the bottleneck here is the > importing of classes, not the number of tests). Ideally we'd just run them > once in a module with a big enough classpath to cover everything. If we > have such a place, that would probably be our best shot. I'll also keep > investigating here, of course. > > For now I just pushed a solution to avoid the overlap when executing it > per-module by matching on the URI. It's not the prettiest solution, but > does work; but that's more to not fail the tests in unrelated modules and > doesn't help much with execution time. > > > Ingo > > On Mon, Sep 6, 2021 at 1:57 PM Chesnay Schepler <ches...@apache.org> > wrote: > >> Do you have an estimate for long these tests would run for? >> >> For project-wide tests, what are the options for setting that up? >> If we let the tests run per-module then I guess they'd overlap >> considerably (because other Flink modules are being put on the >> classpath), which isn't ideal. >> >> On 06/09/2021 13:51, David Morávek wrote: >> > Hi Ingo, >> > >> > +1 for this effort. This could automate lot of "written rules" that are >> > easy to forget about / not to be aware of (such as that each test should >> > extend the TestLogger as Till has already mentioned). >> > >> > I went trough your examples and ArchUnit looks really powerful and >> > expressive while still being easy to read. >> > >> > Best, >> > D. >> > >> > On Mon, Sep 6, 2021 at 1:00 PM Ingo Bürk <i...@ververica.com> wrote: >> > >> >> Thanks for your input Chesnay! >> >> >> >> The limitations of ArchUnit probably mostly stem from the fact that it >> >> operates on byte code and thus can't access anything not accessible >> from >> >> byte code, i.e. JavaDocs. But I think Checkstyle and ArchUnit are >> >> complementing each other quite well here. The main reason against >> >> Checkstyle for these tests is its limitation to single files only, >> >> rendering many tests (including the one you mentioned) impossible. The >> >> secondary reason is that ArchUnit has more declarative APIs and the >> tests >> >> become quite easy to write and maintain (some groundwork effort is >> needed, >> >> of course). Over time we could probably expand quite a bit more on >> what is >> >> tested with ArchUnit as it can test entire architectures (package >> accesses >> >> etc.), and it has support for "freezing" known violations to prevent >> new >> >> violations and removing existing ones over time. >> >> >> >> The @VisibleForTesting use case you mentioned is possible; I've pushed >> a >> >> version of that rule to the draft PR now as well. >> >> >> >> >> >> Best >> >> Ingo >> >> >> >> On Mon, Sep 6, 2021 at 12:11 PM Chesnay Schepler <ches...@apache.org> >> >> wrote: >> >> >> >>> This sounds like an interesting effort. >> >>> >> >>> The draft you have opened uses ArchUnit; can you explain a bit what >> the >> >>> capabilities/limitations of said tool are? >> >>> >> >>> One thing we wanted to have for a long time is that methods/classes >> >>> annotated with @VisibleForTesting are not called from production code; >> >>> would that be something that could be implemented? >> >>> >> >>> It's not a problem imo that tests need to run in order to catch stuff; >> >>> so long as it is noticed on CI. >> >>> >> >>> On 03/09/2021 08:48, Ingo Bürk wrote: >> >>>> Hi Timo, Till, >> >>>> >> >>>> thanks for your input already. I'm glad to hear that the idea >> >> resonates, >> >>>> also thanks for the additional ideas! >> >>>> >> >>>> I've created a JIRA issue[1] for now just to track this idea. I'm >> also >> >>>> working on a bit of a proof of concept and opened it as a draft >> PR[2]. >> >>> I'm >> >>>> happy for anyone to join that PR to look and discuss. The PR doesn't >> >>>> necessarily intend to be merged in its current state, but is rather >> for >> >>>> evaluation. >> >>>> >> >>>> Meanwhile I'm also collecting ideas in a Google Doc, so if anyone >> wants >> >>> to >> >>>> suggest more use cases to explore or implement, please let me know. >> >>>> >> >>>> [1] https://issues.apache.org/jira/browse/FLINK-24138 >> >>>> [2] https://github.com/apache/flink/pull/17133 >> >>>> >> >>>> >> >>>> Best >> >>>> Ingo >> >>>> >> >>>> On Thu, Sep 2, 2021 at 12:31 PM Till Rohrmann <trohrm...@apache.org> >> >>> wrote: >> >>>>> If it is possible to automate these kinds of checks, then I am all >> for >> >>> it >> >>>>> because everything else breaks eventually. So +1 for this proposal. >> >>>>> >> >>>>> I don't have experience with what tools are available, though. >> >>>>> >> >>>>> I would like to add a rule that every test class extends directly or >> >>>>> indirectly TestLogger because otherwise it is super hard to make >> sense >> >>> of >> >>>>> the test logs (Arvid will probably chime in stating that this will >> be >> >>>>> solved with Junit5 eventually). >> >>>>> >> >>>>> Not sure whether this is possible or not but if we can check that >> all >> >>>>> interfaces have properly defined JavaDocs on each method, then this >> >>> could >> >>>>> also be helpful in my opinion. >> >>>>> >> >>>>> Cheers, >> >>>>> Till >> >>>>> >> >>>>> On Thu, Sep 2, 2021 at 11:16 AM Timo Walther <twal...@apache.org> >> >>> wrote: >> >>>>>> Hi Ingo, >> >>>>>> >> >>>>>> thanks for starting this discussion. Having more automation is >> >>>>>> definitely desirable. Esp. in the API / SDK areas where we >> frequently >> >>>>>> have to add similar comments to PRs. The more checks the better. We >> >>>>>> definitely also need more guidelines (e.g. how to develop a Flink >> >>>>>> connector) but automation is safer then long checklists that might >> be >> >>>>>> out of date quickly. >> >>>>>> >> >>>>>> +1 to the proposal. I don't have an opinion on the tool though. >> >>>>>> >> >>>>>> Regards, >> >>>>>> Timo >> >>>>>> >> >>>>>> >> >>>>>> On 01.09.21 11:03, Ingo Bürk wrote: >> >>>>>>> Hello everyone, >> >>>>>>> >> >>>>>>> I would like to start a discussion on introducing automated tests >> >> for >> >>>>>> more >> >>>>>>> architectural rather than stilistic topics. For example, here are >> a >> >>> few >> >>>>>>> things that seem worth checking to me (this is Table-API-focused >> >> since >> >>>>> it >> >>>>>>> is the subsystem I'm involved in): >> >>>>>>> >> >>>>>>> (a) All classes in o.a.f.table.api should be annotated with one >> >>>>>>> of @Internal, @PublicEvolving, or @Public. >> >>>>>>> (b) Classes whose name ends in *ConnectorOptions should be located >> >> in >> >>>>>>> o.a.f.connector.*.table >> >>>>>>> (c) Classes implementing DynamicSourceFactory / DynamicSinkFactory >> >>>>> should >> >>>>>>> have no static members of type ConfigOption >> >>>>>>> >> >>>>>>> There are probably significantly more cases worth checking, and >> also >> >>>>> more >> >>>>>>> involved ones (these are rather simple examples), like disallowing >> >>>>> access >> >>>>>>> between certain packages etc. There are two questions I would like >> >> to >> >>>>> ask >> >>>>>>> to the community: >> >>>>>>> >> >>>>>>> (1) Do you think such tests are useful in general? >> >>>>>>> (2) What use cases come to mind for you? >> >>>>>>> >> >>>>>>> If the idea finds consensus, I would like to use (2) to >> investigate >> >>>>> which >> >>>>>>> tooling to use. An obvious candidate is Checkstyle, as this is >> >> already >> >>>>>>> used. It also has the advantage of being well integrated in the >> IDE. >> >>>>>>> However, it is limited to looking at single files only, and custom >> >>>>> checks >> >>>>>>> are pretty complicated and involved to implement[1]. Another >> >> possible >> >>>>>> tool >> >>>>>>> is ArchUnit[2], which would be significantly easier to maintain >> and >> >> is >> >>>>>> more >> >>>>>>> powerful, but in turn requires tests to be executed. If you have >> >>>>> further >> >>>>>>> suggestions (or thoughts) they would of course also be quite >> >> welcome, >> >>>>>>> though for now I would focus on (1) and (2) and go from there to >> >>>>>> evaluate. >> >>>>>>> [1] https://checkstyle.sourceforge.io/writingchecks.html >> >>>>>>> [2] https://www.archunit.org/ >> >>>>>>> >> >>>>>>> >> >>>>>>> Best >> >>>>>>> Ingo >> >>>>>>> >> >>> >> >>
