Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-4593

DiffRepository tests should fail if XML resources are not in alphabetical order

    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.27.0
    • Component/s: None

      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.

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                julianhyde Julian Hyde
                Reporter:
                julianhyde Julian Hyde
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Time Tracking

                  Estimated:
                  Original Estimate - Not Specified
                  Not Specified
                  Remaining:
                  Remaining Estimate - 0h
                  0h
                  Logged:
                  Time Spent - 40m
                  40m