[
https://issues.apache.org/jira/browse/CALCITE-4593?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17334457#comment-17334457
]
Vladimir Sitnikov edited comment on CALCITE-4593 at 4/28/21, 5:32 AM:
----------------------------------------------------------------------
Julian, thanks for the idea and great clarification. However, would it be
better to just sort the list and give cp ... command in the error message?
"Expected 10 cases out of order" is not really actionable. It does not help
developers to fix the error :(
It is the exact reason I hate Checkstyle's import violation errors: you see it
is something wrong, but you do not know the way to proceed. Luckily we do not
use Checkstyle for imports.
was (Author: vladimirsitnikov):
Julian, thanks for the idea and great clarification. However, would it be
better to jus sort the list and give cp ... command in the error message?
"Expected 10 cases out of order" is not really actionable. It does not help
developers to fix the error :(
It is the exact reason I hate Checkstyle's import violation errors: you see it
is something wrong, but you do not know the way to proceed. Luckily we do not
use Checkstyle for imports.
> 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)
