Use a non-deprecated assertThat, and change several test assertions to use assertThat #14518

thecoop · 2025-04-17T13:32:40Z

The main reason for this PR is using an un-deprecated assertThat by default. I've also updated a few uses of old-style assertions to use assertThat and Matchers for readability and a more descriptive error message. There's obviously loads more that could be updated; this is a few to start with.

thecoop · 2025-04-17T13:33:27Z

lucene/core/src/test/org/apache/lucene/index/TestAddIndexes.java

-            Arrays.stream(c.destDir.listAll())
-                .collect(Collectors.toSet())
-                .containsAll(merge.getMergeInfo().files()));
+        assertFalse(Set.of(c.destDir.listAll()).containsAll(merge.getMergeInfo().files()));


This doesn't easily translate into a matcher, so I've changed to use Set.of to make it a bit more readable

I think the previous version was actually more intuitive?

I don't think so - using a stream from an array to collect into a set, vs just creating a set directly from the array?

So, yes, it's all subjective. The previous version had stream methods over multiple lines - to me that read easier compared to the one-liner now.

dweiss

Thank you for working on cleaning up tests. I still favor assertj assertions over hamcrest - they seem so much cleaner to me. But let's keep things consistent, no worries. This looks good to me.

dweiss · 2025-04-18T07:45:33Z

I'll merge this one later today if there are no objections.

rmuir · 2025-04-18T10:57:12Z

lucene/core/src/test/org/apache/lucene/document/TestDocument.java

@@ -233,7 +245,7 @@ public void testPositionIncrementMultiFields() throws Exception {
    PhraseQuery query = new PhraseQuery("indexed_not_tokenized", "test1", "test2");

    ScoreDoc[] hits = searcher.search(query, 1000).scoreDocs;
-    assertEquals(1, hits.length);
+    assertThat(hits, arrayWithSize(1));


I'm wondering why this is considered an improvement, the previous assert is much more readable

I think error messages are better... I haven't been using hamcrest much so I can't tell for sure - assertj definitely provides more informative error messages. I don't mind either way, even though hamcrest's style is confusing to me too.

I guess I'm confused what could be better about the existing error message, it asserts that array's length is 1, and if its something else, you'll see that number.

So... zero benefit, but at the cost of making the code significantly more difficult to read (which is actually very important when trying to debug the tests)

I prefer the more declarative style of assertThat, and the assertion failure specifies what the problem actually is, rather than 'this singular value is not what is expected'

dweiss · 2025-04-19T15:45:57Z

I have taken another look. The one thing I would revert is replacing assertEquals with hamcrest matchers - it is indeed the same functionality, the same output message and more verbose code. Other changes seem more palatable to me - like array containment cleaned up (and this should result in a better error message, should something wrong happen), assertTrue conditions replaced with equality checks (so that values are reported, not just failed assertion).

dweiss · 2025-04-19T15:47:33Z

lucene/core/src/test/org/apache/lucene/document/TestDocument.java

+    assertThat(doc.getFields(), hasSize(10));
    doc.removeFields("keyword");
-    assertEquals(8, doc.getFields().size());
+    assertThat(doc.getFields(), hasSize(8));


This is an example of changes that don't provide any extra value, I think.

dweiss · 2025-04-19T15:48:02Z

lucene/core/src/test/org/apache/lucene/document/TestDocument.java

@@ -207,7 +219,7 @@ public void testGetValuesForIndexedDocument() throws Exception {

    // ensure that queries return expected results without DateFilter first
    ScoreDoc[] hits = searcher.search(query, 1000).scoreDocs;
-    assertEquals(1, hits.length);
+    assertThat(hits, arrayWithSize(1));


Same here. It's more verbose and does the same thing.

dweiss · 2025-04-19T15:51:47Z

lucene/core/src/test/org/apache/lucene/document/TestDocument.java

+    Set<String> seen = new HashSet<>();
    StoredFields storedFields = searcher.storedFields();
    for (int i = 0; i < 3; i++) {
      Document doc2 = storedFields.document(hits[i].doc);
      Field f = (Field) doc2.getField("id");
-      if (f.stringValue().equals("id1")) result |= 1;
-      else if (f.stringValue().equals("id2")) result |= 2;
-      else if (f.stringValue().equals("id3")) result |= 4;
-      else fail("unexpected id field");
+      switch (f.stringValue()) {
+        case "id1", "id2", "id3" -> seen.add(f.stringValue());
+        default -> fail("unexpected id field");
+      }
    }
    writer.close();
    reader.close();
    dir.close();
-    assertEquals("did not see all IDs", 7, result);
+    assertThat("did not see all IDs", seen, containsInAnyOrder("id1", "id2", "id3"));


I think this could be just assertEquals(seen, Set.of(...)) - this would detect anything extra and check for the occurrence of all ids?

containsInAnyOrder fails if there are any extra items in the collection.

More generally, assertEquals(..., Set.of()) checks if the value is a Set, which is not actually important here. The collection could be a List and the test would still work. So checking for Set isn't required here for the test to pass, whereas containsInAnyOrder doesn't care about the type of collection used.

dweiss · 2025-04-19T15:52:49Z

lucene/core/src/test/org/apache/lucene/index/TestAddIndexes.java

-            Arrays.stream(c.destDir.listAll())
-                .collect(Collectors.toSet())
-                .containsAll(merge.getMergeInfo().files()));
+        assertFalse(Set.of(c.destDir.listAll()).containsAll(merge.getMergeInfo().files()));


I think the previous version was actually more intuitive?

dweiss · 2025-04-19T15:54:20Z

lucene/core/src/test/org/apache/lucene/util/TestArrayUtil.java

+    assertThat(parseInt("1"), equalTo(1));
+    assertThat(parseInt("-10000"), equalTo(-10000));
+    assertThat(parseInt("1923"), equalTo(1923));
+    assertThat(parseInt("-1"), equalTo(-1));
+    assertThat(ArrayUtil.parseInt("foo 1923 bar".toCharArray(), 4, 4), equalTo(1923));


This is an example of a potentially better message but what's wrong with assertEquals(a, b)?

assertEquals is ambiguous to what the thing being test is vs the expected value it should have, assertThat is not

Sure, true, if you're using it the first time perhaps. :)

thecoop · 2025-04-22T13:38:57Z

The focus of this PR was to change assertTrue/assertFalse to assertions that gave what was actually wrong. The assertEquals are things I spotted alongside those.

I prefer the assertThat hamcrest matchers over assertEquals because it is unequivocal what the expected value is vs what is being tested, which is not the case with assertEquals, and I think the matcher assertions read better for what the test is checking, especially with more complicated checks such as arrayWithSize(greaterThan(6)). I used hamcrest as that is used more in the codebase than assertj. However, using hamcrest in this way is a personal preference, that can easily be removed from this PR if people disagree.

dweiss · 2025-04-22T16:43:13Z

I'm fine with merging this change. I don't mind if the code becomes a bit more verbose because of it. Rob?

rmuir · 2025-04-23T09:37:43Z

my concern wasn't about verbosity: if you are concerned about this, steer clear of java!

instead just keeping "barrier to entry" to understanding the tests low. This includes requiring developer to go and understand a third-party library in order to debug it. But if this is really more natural to most developers, then I'm ok.

Change several test assertions to use assertThat

8a2f89d

github-project-automation bot added this to OpenSearch Lucene & Core Performance Tracking Apr 17, 2025

github-project-automation bot moved this to Open in OpenSearch Lucene & Core Performance Tracking Apr 17, 2025

github-actions bot added module:core/index module:core/search module:test-framework labels Apr 17, 2025

thecoop commented Apr 17, 2025

View reviewed changes

dweiss approved these changes Apr 18, 2025

View reviewed changes

rmuir reviewed Apr 18, 2025

View reviewed changes

dweiss reviewed Apr 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a non-deprecated assertThat, and change several test assertions to use assertThat #14518

Use a non-deprecated assertThat, and change several test assertions to use assertThat #14518

thecoop commented Apr 17, 2025 •

edited

Loading

thecoop Apr 17, 2025

dweiss Apr 19, 2025

thecoop Apr 22, 2025

dweiss Apr 22, 2025

dweiss left a comment

dweiss commented Apr 18, 2025

rmuir Apr 18, 2025

dweiss Apr 18, 2025

rmuir Apr 18, 2025

thecoop Apr 22, 2025

dweiss commented Apr 19, 2025

dweiss Apr 19, 2025

dweiss Apr 19, 2025

dweiss Apr 19, 2025

thecoop Apr 23, 2025

dweiss Apr 19, 2025

dweiss Apr 19, 2025

thecoop Apr 22, 2025

dweiss Apr 22, 2025

thecoop commented Apr 22, 2025 •

edited

Loading

dweiss commented Apr 22, 2025

rmuir commented Apr 23, 2025

Use a non-deprecated assertThat, and change several test assertions to use assertThat #14518

Are you sure you want to change the base?

Use a non-deprecated assertThat, and change several test assertions to use assertThat #14518

Conversation

thecoop commented Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dweiss left a comment

Choose a reason for hiding this comment

dweiss commented Apr 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dweiss commented Apr 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thecoop commented Apr 22, 2025 • edited Loading

dweiss commented Apr 22, 2025

rmuir commented Apr 23, 2025

thecoop commented Apr 17, 2025 •

edited

Loading

thecoop commented Apr 22, 2025 •

edited

Loading