You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/05/04 17:42:24 UTC
[GitHub] [calcite] julianhyde opened a new pull request #2409: [CALCITE-4593] DiffRepository tests should fail if new XML resources are not in alphabetical order
julianhyde opened a new pull request #2409:
URL: https://github.com/apache/calcite/pull/2409
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde closed pull request #2409: [CALCITE-4593] DiffRepository tests should fail if new XML resources are not in alphabetical order
Posted by GitBox <gi...@apache.org>.
julianhyde closed pull request #2409:
URL: https://github.com/apache/calcite/pull/2409
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2409: [CALCITE-4593] DiffRepository tests should fail if new XML resources are not in alphabetical order
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2409:
URL: https://github.com/apache/calcite/pull/2409#discussion_r626012880
##########
File path: core/src/test/java/org/apache/calcite/test/DiffRepository.java
##########
@@ -217,9 +235,29 @@ private DiffRepository(
} catch (ParserConfigurationException | SAXException e) {
throw new RuntimeException("error while creating xml parser", e);
}
- indent = logFile.getPath().contains("RelOptRulesTest")
- || logFile.getPath().contains("SqlToRelConverterTest")
- || logFile.getPath().contains("SqlLimitsTest") ? 4 : 2;
+
+ // Test cases should be sorted, but we allow exceptions.
+ // Figure out which test cases are out of order.
+ final List<String> names = new ArrayList<>();
+ for (Node testCase : iterate(root.getElementsByTagName("TestCase"))) {
+ if (testCase instanceof Element) {
+ names.add(((Element) testCase).getAttribute(TEST_CASE_NAME_ATTR));
+ }
+ }
+ final SortedSet<String> outOfOrderTests = TestUtil.outOfOrderItems(names);
+ this.unexpectedOutOfOrderTests =
+ ImmutableSortedSet.copyOf(Ordering.natural(),
+ Sets.difference(outOfOrderTests,
+ new HashSet<>(expectedOutOfOrderTests)));
+ final boolean debug = false;
+ if (debug) {
+ final Set<String> inOrderNames = new TreeSet<>(expectedOutOfOrderTests);
+ inOrderNames.removeAll(outOfOrderTests);
+ if (!inOrderNames.isEmpty()) {
+ throw new IllegalArgumentException("some names were expected to be out "
+ + "of order but were not: " + toLiteralArray(inOrderNames));
Review comment:
Does this ever trigger? What does "names were expected to be out of order" mean? We expect only "in order" names.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2409: [CALCITE-4593] DiffRepository tests should fail if new XML resources are not in alphabetical order
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2409:
URL: https://github.com/apache/calcite/pull/2409#discussion_r625987202
##########
File path: core/src/test/java/org/apache/calcite/test/DiffRepository.java
##########
@@ -756,12 +863,16 @@ public static DiffRepository lookup(
* @param clazz Test case class
* @param baseRepository Base repository
* @param filter Filters each string returned by the repository
+ * @param indent Indent of the XML file (usually 2)
+ * @param expectedOutOfOrderTests List of {@code <TestCase>} elements that
Review comment:
expectedOutOfOrderTests is always 0. Would you please remove the overload to avoid accidental complexity?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2409: [CALCITE-4593] DiffRepository tests should fail if new XML resources are not in alphabetical order
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2409:
URL: https://github.com/apache/calcite/pull/2409#discussion_r625982966
##########
File path: core/src/test/java/org/apache/calcite/test/DiffRepository.java
##########
@@ -517,17 +571,23 @@ private Node ref(String testCaseName, List<Pair<String, Element>> map) {
/**
* Flushes the reference document to the file system.
*/
- private void flushDoc() {
+ private synchronized void flushDoc() {
+ if (modCount == modCountAtLastWrite) {
+ // Document has not been modified since last write.
+ return;
+ }
try {
boolean b = logFile.getParentFile().mkdirs();
Util.discard(b);
try (Writer w = Util.printWriter(logFile)) {
write(doc, w, indent);
}
+ System.out.println("write; modCount=" + modCount);
Review comment:
Please remove system.out before merging
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org