[CALCITE-4593] DiffRepository tests should fail if XML resources are not in alphabetical order

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 1.27.0
• Labels: pull-request-available
• Assignee: Julian Hyde
• Reporter: Julian Hyde
• Created: 28/Apr/21 03:52
• Updated: 03/Jun/21 22:31
• Resolved: 06/May/21 00:05

Description

Tests that use XML resources managed via class DiffRepository should fail if the XML resources are not in alphabetical order.

First, some background. Quite a few tests store resources in an XML file, accessed via class DiffRepository. For example, RelOptRulesTest stores the plan before and after the rule(s) that it is testing are fired. The resources are organized by test case name, enclosed in <TestCase> elements.

Contributors have a tendency to add test resources at the end of the file. But this makes the end of the file a hot-spot for conflicts.

Therefore the best practice is to put the resources into alphabetical order. DiffRepository tries to help with this, by generating an _actual.xml file with the new resource in inserted in its right alphabetical position, but contributors somehow miss this, and write the file by hand. So, conflicts.

With this change, a test will fail if resources are out of order. The message will look something like this:

 java.lang.IllegalArgumentException: expected 10 test cases to be out of order, but there were 11; here are the new ones:
"testAggregateRemove6"
  at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2051)
  at com.google.common.cache.LocalCache.get(LocalCache.java:3951)
  at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:3974)
  at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:4958)
  at com.google.common.cache.LocalCache$LocalLoadingCache.getUnchecked(LocalCache.java:4964)
  at org.apache.calcite.test.DiffRepository.lookup(DiffRepository.java:808)
  at org.apache.calcite.test.RelOptRulesTest.getDiffRepos(RelOptRulesTest.java:190)

Why does it say "expected 10 test case to be out of order, but there were 11"? Because resource files are not currently in total order: they were not sorted to start with, and we don't want to destroy history by sorting them. But to solve the conflict problem, we only need new test cases to be in order. So, this change adds a list of exceptions - test cases that are known to be out of order - and DiffRepository will not complain if those test cases are out of order. Over time, the sort order of resource files will get better, or at least will not get any worse.

Issue Links

• relates to: CALCITE-4595 Split DiffRepository test data into per-test files
• links to: GitHub Pull Request #2409