Modernize BWC testing with parameterized tests #13046

s1monw · 2024-01-29T12:22:28Z

After working on #12829 I was quite surprised about the complexity our BWC have and how ancient the appear compared to the rest of our test suite. I took a step back and tried to modernize the tests to leverage RandomizedRunner Parameterized Tests that allow us to have more structured and hopefully more extendible BWC tests in the future. This change doesn't add any new tests but tries to make the ones we have more structured and support growth down the road.
Basically, every index type got it's own Test class that doesn't require to loop over all the indices in each test. Each test case is run with all versions specified. Several sanity checks are applied in the base class to make individual tests smaller and much easier to read.

s1monw · 2024-01-29T12:27:42Z

...ard-codecs/src/test/org/apache/lucene/backward_index/TestDVUpdateBackwardsCompatibility.java

+  @ParametersFactory(argumentFormatting = "Lucene-Version:%1$s; Pattern: %2$s")
+  public static Iterable<Object[]> testVersionsFactory() {
+    List<Object[]> params = new ArrayList<>();
+    // NOCOMMIT: why are we only testing one version here?


maybe somebody has an answer to that

s1monw · 2024-01-29T12:28:49Z

...codecs/src/test/org/apache/lucene/backward_index/TestIndexUpgradeBackwardsCompatibility.java

+  }
+
+  public void testUpgradeOldSingleSegmentIndexWithAdditions() throws Exception {
+    // NOCOMMIT we use to have single segment indices but we stopped creating them at some point


maybe somebody has an answer to that, this puzzles me as well

I don't know why we stopped! We should start again? But maybe as followon?

jpountz

I like the approach. I'm curious of @dweiss' opinion since he's more familiar with the testing framework.

...kward-codecs/src/test/org/apache/lucene/backward_index/TestBinaryBackwardsCompatibility.java

jpountz · 2024-01-29T12:37:37Z

...d-codecs/src/test/org/apache/lucene/backward_index/TestEmptyIndexBackwardsCompatibility.java

+  @ParametersFactory(argumentFormatting = "Lucene-Version:%1$s; Pattern: %2$s")
+  public static Iterable<Object[]> testVersionsFactory() {
+    List<Object[]> params = new ArrayList<>();
+    // NOCOMMIT: why are we only testing one version here?


I would guess it's because it's more about testing the upgrade logic on empty indexes than about testing the format of old empty indexes.

dweiss · 2024-01-29T15:59:36Z

I like it. Whenever there is test repetition that can be driven by data, it should be driven by data.

The only downside to using ParametersFactory is that it's something that is implemented by the randomizedtesting framework (not JUnit itself) and people may be unfamiliar with it - perhaps a comment on the class (or the parameterized constructor) pointing at where the arguments come from would make it easier to grasp how this thing works?

Plain JUnit alternatives do exist but they rely on a different test runner (Parameterized) - you wouldn't be able to extend from LuceneTestCase anymore (and you'd be outside of the randomization context). I don't see any other, more elegant, solutions.

dweiss · 2024-01-29T16:02:37Z

Oh, one more problem is that you can't "see" the structure of tests before you actually run them (in an IDE). Don't know how much of an issue this is in practice but it's impossible to solve with JUnit4. Some IDEs may also have problems repeating a particular test if its name contains parameters and their names. But I don't think we should think in terms of IDE support - the final outcome and benefit for maintainability matters more.

mikemccand

This looks great! Thank you for modernizing our ancient BWC tests @s1monw. I left minor comments. For the // nocommit questions let's make followon issues to address? We should PnP this!

...codecs/src/test/org/apache/lucene/backward_index/TestIndexUpgradeBackwardsCompatibility.java

mikemccand · 2024-01-29T16:45:16Z

...codecs/src/test/org/apache/lucene/backward_index/TestIndexUpgradeBackwardsCompatibility.java

+  }
+
+  public void testUpgradeOldSingleSegmentIndexWithAdditions() throws Exception {
+    // NOCOMMIT we use to have single segment indices but we stopped creating them at some point


I don't know why we stopped! We should start again? But maybe as followon?

s1monw · 2024-01-30T09:44:07Z

We should PnP this!

What on earth means PnP? Mike, check out this search: https://www.google.com/search?q=pnp+acronym wikipedia FTW

…ex/TestBinaryBackwardsCompatibility.java Co-authored-by: Adrien Grand <[email protected]>

uschindler

Looks fine to me. Theres only a forbiddenapis problem due to String#formatted.

Replace by String.format(Locale.ROOT, indexPattern, version)

...ackward-codecs/src/test/org/apache/lucene/backward_index/BackwardsCompatibilityTestBase.java

mikemccand · 2024-01-30T16:11:21Z

We should PnP this!

What on earth means PnP? Mike, check out this search: https://www.google.com/search?q=pnp+acronym wikipedia FTW

PnP = progress not perfection! Not plug and play, not transistors, heh.

uschindler · 2024-01-30T16:58:52Z

We should PnP this!

What on earth means PnP? Mike, check out this search: https://www.google.com/search?q=pnp+acronym wikipedia FTW

PnP = progress not perfection! Not plug and play, not transistors, heh.

We all need some drugs sometimes, especially the policeman. 💉💊💊💊

s1monw · 2024-01-30T17:08:40Z

@uschindler I need to hear you speaking german to tell, you know that ;)

...ackward-codecs/src/test/org/apache/lucene/backward_index/BackwardsCompatibilityTestBase.java

uschindler

Looks fine. All nocommits are todos now.

I think it's ready for a first round. P💊P

P.S.: About the code style I would prefer the list of versions as a Stream.of(...) and converting them to those sets of versions using a stream mapping. Those array declarations look always strange.

s1monw · 2024-01-31T14:28:19Z

thanks everybody... I will go and backport this to 9.x as well

This change modernizes the BWC tests to leverage RandomizedRunners Parameterized Tests that allow us to have more structured and hopefully more extendible BWC tests in the future. This change doesn't add any new tests but tries to make the ones we have more structured and support growth down the road. Basically, every index type got it's own Test class that doesn't require to loop over all the indices in each test. Each test case is run with all versions specified. Several sanity checks are applied in the base class to make individual tests smaller and much easier to read. Co-authored-by: Michael McCandless <[email protected]> Co-authored-by: Adrien Grand <[email protected]>

Relates to #13046

HoustonPutman · 2024-02-09T16:30:47Z

So I just finished the 8.11.3 release, and had some problems with the back-compat testing code. Obviously the ant/gradle switch and jdk versions meant that I had to do a lot of it manually, but I'm pretty sure the regex matching in the update_backcompat_tests() function in addBackcompatIndexes.py was broken by this commit.

I updated the files myself manually for 8.11.3 (on main and 9x), and I think they are correct (and the tests pass), but I would recommend checking them and re-checking that the python script works with these changes...

In apache#13046 several changes broke the addBackcompatIndexes.py script to properly add and test the unreleased version. This updates the script to again properly add the new version. Closes apache#13094

…3095) In #13046 several changes broke the addBackcompatIndexes.py script to properly add and test the unreleased version. This updates the script to again properly add the new version. Closes #13094 Co-authored-by: Dawid Weiss <[email protected]>

s1monw added 12 commits January 22, 2024 23:11

initial commit

aa76703

cut over more test

139f691

add test to generate bwc indices

47d702b

cut over basic test

e2d5727

add upgrade test

fd64abe

add test for empty index

e7ed6bf

commit progress

e046b29

tidy

2081129

clean up dead code

9f667f2

move last tests

5a1c1f5

fix bwc indices gerenation script

f582b05

tidy

e1c871a

s1monw requested review from dweiss, jpountz and mikemccand January 29, 2024 12:22

s1monw commented Jan 29, 2024

View reviewed changes

jpountz reviewed Jan 29, 2024

View reviewed changes

mikemccand approved these changes Jan 29, 2024

View reviewed changes

s1monw and others added 4 commits January 30, 2024 11:06

Merge branch 'main' into new_bwc_tests

00ef07b

use the provided directory

2b65133

move no-commits to todos

eb88593

Update lucene/backward-codecs/src/test/org/apache/lucene/backward_ind…

8271fde

…ex/TestBinaryBackwardsCompatibility.java Co-authored-by: Adrien Grand <[email protected]>

s1monw force-pushed the new_bwc_tests branch from 8ce6014 to 8271fde Compare January 30, 2024 10:37

tidy

e414212

uschindler requested changes Jan 30, 2024

View reviewed changes

...ackward-codecs/src/test/org/apache/lucene/backward_index/BackwardsCompatibilityTestBase.java Outdated Show resolved Hide resolved

make policman happy

b7f57db

s1monw requested a review from uschindler January 30, 2024 17:09

uschindler requested changes Jan 30, 2024

View reviewed changes

...ackward-codecs/src/test/org/apache/lucene/backward_index/BackwardsCompatibilityTestBase.java Outdated Show resolved Hide resolved

fix string formatting

714c8e8

s1monw requested a review from uschindler January 30, 2024 19:44

uschindler approved these changes Jan 30, 2024

View reviewed changes

dweiss approved these changes Jan 30, 2024

View reviewed changes

s1monw merged commit 8d9290c into apache:main Jan 31, 2024
4 checks passed

s1monw added the type:test label Jan 31, 2024

s1monw added a commit that referenced this pull request Jan 31, 2024

Ensure test is only run on the previous major

1a6932f

Relates to #13046

dweiss mentioned this pull request Feb 5, 2024

java.lang.AssertionError in backward compat tests ("failed to parse ... as date") #13073

Closed

dweiss mentioned this pull request Feb 11, 2024

Python backward compat scripts need updating #13094

Closed

s1monw mentioned this pull request Feb 12, 2024

Fix addBackcompatIndexes.py to properly generate missing versions #13095

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize BWC testing with parameterized tests #13046

Modernize BWC testing with parameterized tests #13046

s1monw commented Jan 29, 2024 •

edited

Loading

s1monw Jan 29, 2024

s1monw Jan 29, 2024

mikemccand Jan 29, 2024

jpountz left a comment

jpountz Jan 29, 2024

dweiss commented Jan 29, 2024

dweiss commented Jan 29, 2024

mikemccand left a comment

mikemccand Jan 29, 2024

s1monw commented Jan 30, 2024

uschindler left a comment

mikemccand commented Jan 30, 2024

uschindler commented Jan 30, 2024

s1monw commented Jan 30, 2024

uschindler left a comment

s1monw commented Jan 31, 2024

HoustonPutman commented Feb 9, 2024

Modernize BWC testing with parameterized tests #13046

Modernize BWC testing with parameterized tests #13046

Conversation

s1monw commented Jan 29, 2024 • edited Loading

s1monw Jan 29, 2024

Choose a reason for hiding this comment

s1monw Jan 29, 2024

Choose a reason for hiding this comment

mikemccand Jan 29, 2024

Choose a reason for hiding this comment

jpountz left a comment

Choose a reason for hiding this comment

jpountz Jan 29, 2024

Choose a reason for hiding this comment

dweiss commented Jan 29, 2024

dweiss commented Jan 29, 2024

mikemccand left a comment

Choose a reason for hiding this comment

mikemccand Jan 29, 2024

Choose a reason for hiding this comment

s1monw commented Jan 30, 2024

uschindler left a comment

Choose a reason for hiding this comment

mikemccand commented Jan 30, 2024

uschindler commented Jan 30, 2024

s1monw commented Jan 30, 2024

uschindler left a comment

Choose a reason for hiding this comment

s1monw commented Jan 31, 2024

HoustonPutman commented Feb 9, 2024

s1monw commented Jan 29, 2024 •

edited

Loading