Hi Chris! > [ not a review ]
[feedback is always welcome, especially from you] > I think that the approach is reasonable (to inject ’test’ into the package > name between the reverse DNS prefix and the specific logical technology > suffix). It's actually '.tests'... Robert (Muir) suggested .test as well but I must have added that extra 's' somewhere along the line and I didn't have the heart to redo all the refactorings... > I’ve played a little with similar in the Elasticsearch codebase - but not yet > advanced it to a point that eliminates all split packages. ES is much larger and more complex, so I'm not surprised. Same like Solr. Lucene is relatively simple -- lots of code but all of the modules are fairly simple structure-wise. Makes such experiments more approachable. > TestSecrets also seems reasonable. It’s a pity that it intrudes somewhat on > the actual product code, but not to any extent that would be concerning. This is quite funny - I actually reengineered the internal-package trick, although my version looked slightly different (it had public methods returning the "secret" accessors, although they couldn't be instantiated by anything else than the internal package - you can see it in the commit history... then I recalled that the JDK had a similar solution and peeked at how that was done. I thought it was nicer as it didn't pollute the API. So now you know whom I borrowed the idea from - you. :) The only difference is the use of unsafe to make sure classes have their static blocks invoked. I didn't want to bring unsafe into the mix and used Class.forName(apiClazz.getName()). I don't think this can have any side-effects whatsoever and the contract on this variant of forName has the initialization flag on, so it seems to be safe - correct me if I'm wrong though. > It’s great that you can now have a test_framework module. And, from what I > can see, the moduleXXX configurations that you introduced recently work just > fine for the Gradle dependency configuration. I have to admit this works rather nicely. There are some rough corners we discovered already but I think they're all fixable with relative easy - https://issues.apache.org/jira/browse/LUCENE-10328 I'm sure in the end this could be extracted into a more reusable gradle plugin, probably replacing the built-in support for modules, but for the time being it's just easier to work with those separate gradle scripts. > Once this is in, then it will be possible to patch area specific unit tests > into the actual product module and `require` the framework, right? Yes, I think this is doable. It's what Uwe has been asking for - his panama branch currently requires tricks that shouldn't be there. > And if we had that, it’s not a big leap to maybe refactor some of the secrets > to be injected too ( but I accept that that is not really necessary either, > and I’m not sure how the IDEs or Gradle would like this ) What secrets do you have in mind here? As for IDEs: IntelliJ should work fine as it converts each source set into a logical dependency unit. I don't think how Eclipse can be made to digest all this - for now, we create a big bag of sources without submodule demarcation. So it compiles, although is far from ideal. Dawid --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
