zabetak commented on code in PR #4597:
URL: https://github.com/apache/calcite/pull/4597#discussion_r2459465966
##########
testkit/src/main/java/org/apache/calcite/test/DiffRepository.java:
##########
@@ -255,6 +255,15 @@ private DiffRepository(URL refFile, File logFile,
//~ Methods ----------------------------------------------------------------
public void checkActualAndReferenceFiles() {
+ // Check if any test cases are out of order FIRST, before other checks
+ // This ensures that we fail fast if there are ordering issues
+ if (!outOfOrderTests.isEmpty()) {
+ throw new IllegalArgumentException("TestCase(s) are out of alphabetical
order in the"
+ + " reference file: " + Sources.of(refFile).file() + "\n"
+ + "Out-of-order test cases: " + outOfOrderTests + "\n"
+ + "To fix, copy the generated log file: " + logFile);
+ }
+
Review Comment:
Why do we need this check here? Aren't the remaining checks sufficient to
detect that the files are not identical? Can't we follow the same pattern with
the check below:
```java
if(existsMethodOnlyInXml || !outOfOrderTests.isEmpty()) {
modCount++;
flushDoc();
}
```
##########
testkit/src/test/java/org/apache/calcite/test/DiffRepositoryTest.java:
##########
@@ -57,4 +57,21 @@ public class DiffRepositoryTest {
}
assertThat("First assertion must always fail", assertPassed, is(false));
}
+
+ @Test void testOutOfOrderTestCases() throws Exception {
+ DiffRepository r =
DiffRepository.lookup(DiffRepositoryTestOutOfOrder.class);
+
+ IllegalArgumentException thrown = null;
+ try {
+ r.checkActualAndReferenceFiles();
+ } catch (IllegalArgumentException e) {
+ thrown = e;
+ }
+
+ // We expect an error about out-of-order test cases
+ assertThat("checkActualAndReferenceFiles should throw an error for
out-of-order tests",
+ thrown != null, is(true));
+ assertThat("Error message should mention out-of-order or alphabetical",
Review Comment:
If we get a specific "out-of-order" error when calling
`DiffRepository#assertEquals` then we can keep the existing generic error
message in `checkActualAndReferenceFiles`. In most cases, we are hitting the
code of `DiffRepository#getTestCaseElement` first before we reach
`checkActualAndReferenceFiles`.
##########
testkit/src/test/java/org/apache/calcite/test/DiffRepositoryTest.java:
##########
@@ -57,4 +57,21 @@ public class DiffRepositoryTest {
}
assertThat("First assertion must always fail", assertPassed, is(false));
}
+
+ @Test void testOutOfOrderTestCases() throws Exception {
+ DiffRepository r =
DiffRepository.lookup(DiffRepositoryTestOutOfOrder.class);
+
+ IllegalArgumentException thrown = null;
+ try {
+ r.checkActualAndReferenceFiles();
Review Comment:
People who implement tests based on DiffRepository may not always call the
`checkActualAndReferenceFiles`. Can we add some unit tests that ensure that we
capture the ordering problem when `DiffRepository#assertEquals` is used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
