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