Re: [DISCUSS] Automated architectural tests

2021-11-24 Thread Ingo Bürk
Hi everyone, I'm happy to announce now that we finished this test and it has been merged (thanks to Chesnay for his help here). You can also find some documentation for it here[1]. I haven't implemented all of the suggested rules, but we can of course expand our checks here. The current rules

Re: [DISCUSS] Automated architectural tests

2021-09-10 Thread Ingo Bürk
Thanks, JING ZHANG. The first one is definitely doable, I will add it to my list. The second one I'll have to take a look at. On Fri, Sep 10, 2021 at 2:27 PM JING ZHANG wrote: > Hi Ingo, > Thanks for driving this discussion. > Some use cases come to my mind, maybe those rules could be checked

Re: [DISCUSS] Automated architectural tests

2021-09-10 Thread JING ZHANG
Hi Ingo, Thanks for driving this discussion. Some use cases come to my mind, maybe those rules could be checked by the same way. 1. new introduced `StreamExecNode` is implemented json serialization/deserialization. Currently it is checked in `JsonSerdeCoverageTest`. 2. new introduced `RelNode`

Re: [DISCUSS] Automated architectural tests

2021-09-09 Thread Ingo Bürk
Great! I'll work on getting the PR into an actual, proper shape now, including looking at found violations more carefully and eventually freezing current violations (maybe removing some quick-wins). One more thing I just ran into is that ArchUnit doesn't explicitly support Scala; while many

Re: [DISCUSS] Automated architectural tests

2021-09-07 Thread Chesnay Schepler
I would say that's fine time-wise. On 07/09/2021 15:29, Ingo Bürk wrote: Thanks, Chesnay. I updated the PR to use a separate module now, and ran it on a few modules (some Table API modules and a couple connectors). The CI seemed to take ~2.5min for executing the tests; that's certainly not

Re: [DISCUSS] Automated architectural tests

2021-09-07 Thread Ingo Bürk
Thanks, Chesnay. I updated the PR to use a separate module now, and ran it on a few modules (some Table API modules and a couple connectors). The CI seemed to take ~2.5min for executing the tests; that's certainly not negligible. On the other hand, even the few tests implemented already found

Re: [DISCUSS] Automated architectural tests

2021-09-06 Thread Chesnay Schepler
While flink-tests is currently the best choice in that it has the biggest classpath, it is also the module already requiring the most time on CI. Furthermore, given that we ideally cover all APIs (including connectors & formats), having that mess of dependencies in flink-tests may interfere

Re: [DISCUSS] Automated architectural tests

2021-09-06 Thread Ingo Bürk
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

Re: [DISCUSS] Automated architectural tests

2021-09-06 Thread Ingo Bürk
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

Re: [DISCUSS] Automated architectural tests

2021-09-06 Thread Chesnay Schepler
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

Re: [DISCUSS] Automated architectural tests

2021-09-06 Thread David Morávek
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

Re: [DISCUSS] Automated architectural tests

2021-09-06 Thread Ingo Bürk
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

Re: [DISCUSS] Automated architectural tests

2021-09-06 Thread Chesnay Schepler
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

Re: [DISCUSS] Automated architectural tests

2021-09-03 Thread Ingo Bürk
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

Re: [DISCUSS] Automated architectural tests

2021-09-02 Thread Till Rohrmann
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

Re: [DISCUSS] Automated architectural tests

2021-09-02 Thread Timo Walther
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

[DISCUSS] Automated architectural tests

2021-09-01 Thread Ingo Bürk
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