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