Mailing List Archive

[Heads up] Test framework package rename
Hello everyone,

I've completed the task of getting the test framework to not share any
packages with Lucene core - this is here:

https://issues.apache.org/jira/browse/LUCENE-10301
https://github.com/apache/lucene/pull/551

Basically everything remains the same, except for the changed package
prefix. The patch is rather large because it cuts across all of the code
(imports, mostly). There are also some minor tweaks to expose
package-private internals in the core to the test framework, now residing
in a different package.

Tests pass for me but I could use a pair of eyes on the patch. This will be
rather annoying for backports (if you change anything in the test framework
itself) so I'd like to apply it to 9x and main. Soon-ish, if there are no
objections.

Dawid
Re: [Heads up] Test framework package rename [ In reply to ]
Thanks for doing this!! Give me a few minutes, I'll verify everything
passes on linux, and review it locally. And I agree, sooner than later
is better as it will conflict with anything and everything.

Due to the expected noise of the import statements, the change is so
large that it makes the Github UI unresponsive, so I'll review the
diff locally while check runs.

On Mon, Dec 20, 2021 at 9:55 AM Dawid Weiss <dawid.weiss@gmail.com> wrote:
>
>
> Hello everyone,
>
> I've completed the task of getting the test framework to not share any packages with Lucene core - this is here:
>
> https://issues.apache.org/jira/browse/LUCENE-10301
> https://github.com/apache/lucene/pull/551
>
> Basically everything remains the same, except for the changed package prefix. The patch is rather large because it cuts across all of the code (imports, mostly). There are also some minor tweaks to expose package-private internals in the core to the test framework, now residing in a different package.
>
> Tests pass for me but I could use a pair of eyes on the patch. This will be rather annoying for backports (if you change anything in the test framework itself) so I'd like to apply it to 9x and main. Soon-ish, if there are no objections.
>
> Dawid

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: [Heads up] Test framework package rename [ In reply to ]
> large that it makes the Github UI unresponsive, so I'll review the
> diff locally while check runs.
>

Right - I did the same, actually. You can grep/throw away most of the lines
that have imports in them as these are irrelevant.

D.
Re: [Heads up] Test framework package rename [ In reply to ]
Hi David,

[ not a review ]

> On 20 Dec 2021, at 14:54, Dawid Weiss <dawid.weiss@gmail.com> wrote:
>
>
> Hello everyone,
>
> I've completed the task of getting the test framework to not share any packages with Lucene core - this is here:
>
> https://issues.apache.org/jira/browse/LUCENE-10301 <https://issues.apache.org/jira/browse/LUCENE-10301>
> https://github.com/apache/lucene/pull/551 <https://github.com/apache/lucene/pull/551>
>
> Basically everything remains the same, except for the changed package prefix. The patch is rather large because it cuts across all of the code (imports, mostly). There are also some minor tweaks to expose package-private internals in the core to the test framework, now residing in a different package.


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). I’ve played a little with similar in the Elasticsearch codebase - but not yet advanced it to a point that eliminates all split packages.

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.

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 wish that we were at a similar point in ES, but recent distractions have slowed progress :-( )

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

-Chris.
Re: [Heads up] Test framework package rename [ In reply to ]
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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: [Heads up] Test framework package rename [ In reply to ]
Hi David,

> On 20 Dec 2021, at 17:12, Dawid Weiss <dawid.weiss@gmail.com> wrote:
>
> [snip]
>
>> 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...

Ah yes, '.tests’ - which is also fine.

> [ snip ]
>> 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.

Right, if you need access to package-privates from an “API” package,
then secrets are a fine tool for the job.

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

You are correct (you’re safe). Lucene has bumped the minimum JDK
to 17, right? If so, you could use MethodHandles.Lookup::ensureInitialized.
Or this could be done separately, if there is interest. (hmm.. maybe you
don’t wanna go down this rabbit hole now!, lookup access could be an
issue )

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

Yeah, it seems so.

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

Ok, great. I think we’re on the same track.

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

Nothing specific, just that if we’re injecting code into the module
(patching the module for unit testing purposes), then even the secrets
(currently in the product code), could be effectively injected too.
(The code that sets things up in the API packages). Almost as if each
module had its own mini-test-framework, which could be (but does not
strictly need to be) part of its unit test.

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

I was mostly thinking of how the IDE would react to —-patch-module, and
whether or not it could handle things like auto-complete, etc, from
consuming code whose view point just sees the patched module as one
complete unit.

Anyway, that is a distraction from the good work here, and something for
another day.

-Chris.



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: [Heads up] Test framework package rename [ In reply to ]
> You are correct (you’re safe). Lucene has bumped the minimum JDK
> to 17, right? If so, you could use MethodHandles.Lookup::ensureInitialized.

It'll probably go into the 9x branch which will remain on older JDKs. That
Class.forName trick is so simple and straightforward that I think I'm going to
stick to it for now.

> Nothing specific, just that if we’re injecting code into the module
> (patching the module for unit testing purposes), then even the secrets
> (currently in the product code), could be effectively injected too.

Right, I get you now. Yes - indeed it could be completely separate but then
everyone using the test framework module (it is published as part of
Lucene artifacts)
would have to remember and somehow apply the patch options... I'd
rather just go for
simplicity here - if you require the lucene framework module, it'll
just work, no
patching needed. As much as I like playing with the jms, the effort
required to get the
tools to a reasonably working state is still significant.

> I was mostly thinking of how the IDE would react to —-patch-module, and
> whether or not it could handle things like auto-complete, etc, from
> consuming code whose view point just sees the patched module as one
> complete unit.

I doubt it'll work the way you describe it. But hey, it's Christmas
time - excellent
time for good wishes. ;)

D.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: [Heads up] Test framework package rename [ In reply to ]
Hi,

On Mon, 20 Dec 2021 at 17:24, Robert Muir <rcmuir@gmail.com> wrote:

> Thanks for doing this!! Give me a few minutes, I'll verify everything
> passes on linux, and review it locally. And I agree, sooner than later
> is better as it will conflict with anything and everything.
>
> Due to the expected noise of the import statements, the change is so
> large that it makes the Github UI unresponsive, so I'll review the
> diff locally while check runs.
>

You can use
https://patch-diff.githubusercontent.com/raw/apache/lucene/pull/551.diff to
render plain text and keep the browser responsive.
Another option is
https://patch-diff.githubusercontent.com/raw/apache/lucene/pull/551.patch
to see each commit separately.


>
> On Mon, Dec 20, 2021 at 9:55 AM Dawid Weiss <dawid.weiss@gmail.com> wrote:
> >
> >
> > Hello everyone,
> >
> > I've completed the task of getting the test framework to not share any
> packages with Lucene core - this is here:
> >
> > https://issues.apache.org/jira/browse/LUCENE-10301
> > https://github.com/apache/lucene/pull/551
> >
> > Basically everything remains the same, except for the changed package
> prefix. The patch is rather large because it cuts across all of the code
> (imports, mostly). There are also some minor tweaks to expose
> package-private internals in the core to the test framework, now residing
> in a different package.
> >
> > Tests pass for me but I could use a pair of eyes on the patch. This will
> be rather annoying for backports (if you change anything in the test
> framework itself) so I'd like to apply it to 9x and main. Soon-ish, if
> there are no objections.
> >
> > Dawid
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
>
Re: [Heads up] Test framework package rename [ In reply to ]
On Tue, 21 Dec 2021 at 15:19, Mark Jens <mark.r.jens@gmail.com> wrote:

> Hi,
>
> On Mon, 20 Dec 2021 at 17:24, Robert Muir <rcmuir@gmail.com> wrote:
>
>> Thanks for doing this!! Give me a few minutes, I'll verify everything
>> passes on linux, and review it locally. And I agree, sooner than later
>> is better as it will conflict with anything and everything.
>>
>> Due to the expected noise of the import statements, the change is so
>> large that it makes the Github UI unresponsive, so I'll review the
>> diff locally while check runs.
>>
>
> You can use
> https://patch-diff.githubusercontent.com/raw/apache/lucene/pull/551.diff
> to render plain text and keep the browser responsive.
> Another option is
> https://patch-diff.githubusercontent.com/raw/apache/lucene/pull/551.patch
> to see each commit separately.
>

To make it more clear: just append '.diff' or '.patch' to
https://github.com/apache/lucene/pull/551 and it will redirect to the above
links.


>
>
>>
>> On Mon, Dec 20, 2021 at 9:55 AM Dawid Weiss <dawid.weiss@gmail.com>
>> wrote:
>> >
>> >
>> > Hello everyone,
>> >
>> > I've completed the task of getting the test framework to not share any
>> packages with Lucene core - this is here:
>> >
>> > https://issues.apache.org/jira/browse/LUCENE-10301
>> > https://github.com/apache/lucene/pull/551
>> >
>> > Basically everything remains the same, except for the changed package
>> prefix. The patch is rather large because it cuts across all of the code
>> (imports, mostly). There are also some minor tweaks to expose
>> package-private internals in the core to the test framework, now residing
>> in a different package.
>> >
>> > Tests pass for me but I could use a pair of eyes on the patch. This
>> will be rather annoying for backports (if you change anything in the test
>> framework itself) so I'd like to apply it to 9x and main. Soon-ish, if
>> there are no objections.
>> >
>> > Dawid
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>> For additional commands, e-mail: dev-help@lucene.apache.org
>>
>>
Re: [Heads up] Test framework package rename [ In reply to ]
On Tue, Dec 21, 2021 at 8:20 AM Mark Jens <mark.r.jens@gmail.com> wrote:
>
> You can use https://patch-diff.githubusercontent.com/raw/apache/lucene/pull/551.diff to render plain text and keep the browser responsive.
> Another option is https://patch-diff.githubusercontent.com/raw/apache/lucene/pull/551.patch to see each commit separately.
>

Thank you Mark.

This does work easily for a large diff, and I get colored diff if I
load it in vim (syntax on/set background=dark).
But personally, especially for such a large diff, I get a much better
color scheme for reviewing via 'git diff'.
Seems I need to tweak the vim colors better for "downloaded diff" to
be more useful...

In any case, fetching Dawid's remote branch and testing it out myself
was worth the trouble. It found a bug
(https://issues.apache.org/jira/browse/LUCENE-10331)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: [Heads up] Test framework package rename [ In reply to ]
Hi,
TortoiseGit's Viewer for diffs also helps well. But I just quicky skimmed through it and only stopped at classes where different changes to imports were done.

Thanks,
Uwe

Am 21. Dezember 2021 14:01:15 UTC schrieb Robert Muir <rcmuir@gmail.com>:
>On Tue, Dec 21, 2021 at 8:20 AM Mark Jens <mark.r.jens@gmail.com> wrote:
>>
>> You can use https://patch-diff.githubusercontent.com/raw/apache/lucene/pull/551.diff to render plain text and keep the browser responsive.
>> Another option is https://patch-diff.githubusercontent.com/raw/apache/lucene/pull/551.patch to see each commit separately.
>>
>
>Thank you Mark.
>
>This does work easily for a large diff, and I get colored diff if I
>load it in vim (syntax on/set background=dark).
>But personally, especially for such a large diff, I get a much better
>color scheme for reviewing via 'git diff'.
>Seems I need to tweak the vim colors better for "downloaded diff" to
>be more useful...
>
>In any case, fetching Dawid's remote branch and testing it out myself
>was worth the trouble. It found a bug
>(https://issues.apache.org/jira/browse/LUCENE-10331)
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>For additional commands, e-mail: dev-help@lucene.apache.org
>

--
Uwe Schindler
Achterdiek 19, 28357 Bremen
https://www.thetaphi.de
Re: [Heads up] Test framework package rename [ In reply to ]
I wouldn't have thought to do such a change in a minor release, but I
suppose for tests it's fine.

~ David Smiley
Apache Lucene/Solr Search Developer
http://www.linkedin.com/in/davidwsmiley


On Tue, Dec 21, 2021 at 9:10 AM Uwe Schindler <uwe@thetaphi.de> wrote:

> Hi,
> TortoiseGit's Viewer for diffs also helps well. But I just quicky skimmed
> through it and only stopped at classes where different changes to imports
> were done.
>
> Thanks,
> Uwe
>
> Am 21. Dezember 2021 14:01:15 UTC schrieb Robert Muir <rcmuir@gmail.com>:
>>
>> On Tue, Dec 21, 2021 at 8:20 AM Mark Jens <mark.r.jens@gmail.com> wrote:
>>
>>>
>>> You can use https://patch-diff.githubusercontent.com/raw/apache/lucene/pull/551.diff to render plain text and keep the browser responsive.
>>> Another option is https://patch-diff.githubusercontent.com/raw/apache/lucene/pull/551.patch to see each commit separately.
>>>
>>>
>> Thank you Mark.
>>
>> This does work easily for a large diff, and I get colored diff if I
>> load it in vim (syntax on/set background=dark).
>> But personally, especially for such a large diff, I get a much better
>> color scheme for reviewing via 'git diff'.
>> Seems I need to tweak the vim colors better for "downloaded diff" to
>> be more useful...
>>
>> In any case, fetching Dawid's remote branch and testing it out myself
>> was worth the trouble. It found a bug
>> (https://issues.apache.org/jira/browse/LUCENE-10331)
>> ------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>> For additional commands, e-mail: dev-help@lucene.apache.org
>>
>> --
> Uwe Schindler
> Achterdiek 19, 28357 Bremen
> https://www.thetaphi.de
>
Re: [Heads up] Test framework package rename [ In reply to ]
> I wouldn't have thought to do such a change in a minor release, but I suppose for tests it's fine.

I think the benefits may outweigh the inconvenience in this case. It'd
be a killer for us in the long run - all the tests would require a
manual touch in backports. I don't see how this can be made to work in
any other way
(*)...

D.

(*) I'm fairly sure Uwe can write a script that will process all the
test framework classes via asmlib and repackage it to the old name
space but I don't think we should do it. ;)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org