[
https://issues.apache.org/jira/browse/CALCITE-4593?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17334639#comment-17334639
]
Stamatis Zampetakis commented on CALCITE-4593:
----------------------------------------------
I think it is worth sorting the file once and keeping it sorted afterwards.
This will make the exception message more intuitive and I guess the code for
this change simpler. Most of the time we need to compare with the last version
of the resource file (and not with some in an older commit) so retaining the
history does not seem to be a big plus for this case.
> DiffRepository tests should fail if XML resources are not in alphabetical
> order
> -------------------------------------------------------------------------------
>
> Key: CALCITE-4593
> URL: https://issues.apache.org/jira/browse/CALCITE-4593
> Project: Calcite
> Issue Type: Bug
> Reporter: Julian Hyde
> Assignee: Julian Hyde
> Priority: Major
> Fix For: 1.27.0
>
>
> 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:
> {noformat}
> 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){noformat}
> 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.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
