Fix test false postives due to TestNG's #543 #7204

ArturGajowy · 2017-01-23T08:08:14Z

Because of TestNG #543 - Unexpected Behaviour: assertEquals for Iterable, ~20 tests are giving false positive results (all due to equality checks on MaterializedResult instances, which implement Iterable and because of that the 'types' field was not taken into account).

The error could potentially occur in ~30 test classes. All potential and actual false positives are fixed and a static analysis check is added to prevent further breakage.

Even if it seems that no real bugs snuck through the false positives, the fixed tests are made to assert the status quo - so extra attention should be given to changes in their source.

losipiuk · 2017-01-23T10:15:12Z

pom.xml

@@ -881,6 +881,7 @@
                        <javaVersion>1.8</javaVersion>
                        <failOnViolations>true</failOnViolations>
                        <exclusionsFile>${air.main.basedir}/src/modernizer/exclusions.txt</exclusionsFile>
+                        <violationsFile>${air.main.basedir}/src/modernizer/violations.xml</violationsFile>


Add comment above <version>1.4.0</version> that violationsFile that violations file needs to be updated to match new version.
If we cannot do better.

losipiuk · 2017-01-23T10:17:29Z

src/modernizer/violations.xml

@@ -0,0 +1,769 @@
+<?xml version="1.0"?>


Split this to commit which imports original violations.xml and one which modifies it.

electrum · 2017-01-23T16:56:57Z

Change all the commit message subjects to be "by TestNG bug" since GitHub wants to link to a Presto issue.

electrum · 2017-01-23T17:02:18Z

Instead of copy/pasting the default violations, we should update the modernizer plugin to support additional files. I submitted a PR the other day and he merged it quickly.

electrum

See comments

ArturGajowy · 2017-01-25T01:23:51Z

@electrum comments addressed. modernizer 1.5.0 has been released, including the needed changes. The build should pass now.

kokosing

It is nice that you have found it! I don't understand why testng is so popular.

kokosing · 2017-01-25T08:15:58Z

pom.xml

@@ -875,12 +875,16 @@
                <plugin>


I think you should first fix tests and then add violations. That way every commit has a chance to build. So please reorder commits.

I agree all commits should build when merged to master, so that bisects work. I introduced the changes in this order so that I see the static checks I introduced catch anything. I left it this way so far so that anyone can easily re-verify it. Will now rearrange the commits.

kokosing · 2017-01-25T08:16:28Z

src/modernizer/violations.xml

+    <comment>Use com.facebook.presto.testing.assertions.Assert.assertEquals due to TestNG #543</comment>
+  </violation>
+
+</modernizer>


now new line at the end

kokosing · 2017-01-25T08:23:38Z

presto-main/pom.xml

@@ -267,7 +267,7 @@
        <dependency>
            <groupId>org.testng</groupId>
            <artifactId>testng</artifactId>
-            <scope>test</scope>
+            <scope>provided</scope>


I don't know why it is changed?

Because the com.facebook.presto.testing.assertions.Assert class depends on TestNG's assertions, and it has to be in src/main so that other modules can use it

kokosing · 2017-01-25T08:25:29Z

presto-main/src/test/java/com/facebook/presto/operator/TestHashJoinOperator.java

@@ -116,7 +116,7 @@ public void testInnerJoin(boolean parallelBuild, boolean probeHashEnabled, boole
        );

        // expected
-        MaterializedResult expected = MaterializedResult.resultBuilder(taskContext.getSession(), concat(probePages.getTypes(), buildPages.getTypes()))
+        MaterializedResult expected = MaterializedResult.resultBuilder(taskContext.getSession(), concat(probePages.getTypesWithoutHash(), buildPages.getTypesWithoutHash()))


can you please squash this commit with the previous one?

But why? I wanted the changes to be atomic. Also I think getTypesWithoutHash method addition is worth mentioning in the commit log for the ones reading it :)

ArturGajowy · 2017-01-25T16:10:24Z

Rebased and addressed @kokosing's comments

ArturGajowy · 2017-01-26T18:18:40Z

@electrum rebased on master

ArturGajowy · 2017-02-03T17:29:56Z

@electrum kindly ping

electrum · 2017-02-11T00:52:52Z

Regarding this sentence in one of the commit descriptions: "Jto address diffrences between cassandra and other connectors."

"differences" has a typo. I'm assuming "Jto" is a typo as well.

electrum · 2017-02-11T00:59:06Z

presto-main/src/main/java/com/facebook/presto/testing/assertions/Assert.java

+
+public class Assert extends org.testng.Assert
+{
+    private Assert()


Format like

private Assert() {}

electrum · 2017-02-11T00:59:16Z

presto-main/src/main/java/com/facebook/presto/testing/assertions/Assert.java

+ */
+package com.facebook.presto.testing.assertions;
+
+public class Assert extends org.testng.Assert


extends should go on next line

electrum · 2017-02-11T00:59:23Z

presto-main/src/main/java/com/facebook/presto/testing/assertions/Assert.java

+ */
+package com.facebook.presto.testing.assertions;
+
+public class Assert extends org.testng.Assert


Add

@SuppressWarnings({"MethodOverridesStaticMethodOfSuperclass", "ExtendsUtilityClass"})

electrum · 2017-02-11T00:59:48Z

presto-main/src/main/java/com/facebook/presto/testing/assertions/Assert.java

+ */
+package com.facebook.presto.testing.assertions;
+
+public class Assert extends org.testng.Assert


Add a comment this class about why it is here

ArturGajowy · 2017-02-13T14:22:50Z

@electrum comments applied, back to you :)

ArturGajowy · 2017-02-20T14:23:07Z

@electrum rebased, kindly ping :)

ArturGajowy · 2017-02-28T15:34:16Z

@electrum kindly ping

See testng-team/testng#543.

Extract parameterized test methods in AbstractTestQueries to address differences between cassandra and other connectors. See testng-team/testng#543.

See testng-team/testng#543.

This prevents false positive test results when comparing iterables (most notably: MaterializedResult instances) due to TestNG bug (testng-team/testng#543). False-positive results were found in ~20 tests and are fixed in the following commits.

ArturGajowy · 2017-03-08T12:42:59Z

@electrum rebased and resolved conflicts. Please have a look, all comments are addressed so far.

ArturGajowy · 2017-03-16T15:45:25Z

@electrum kindly bump.

electrum · 2017-04-07T22:58:42Z

Merged, thanks!

facebook-github-bot added the CLA Signed label Jan 23, 2017

ArturGajowy requested review from losipiuk and kokosing January 23, 2017 10:00

losipiuk suggested changes Jan 23, 2017

View reviewed changes

ArturGajowy force-pushed the fix_test_false_postives_testng_543 branch 2 times, most recently from 2556f14 to 472e2d4 Compare January 23, 2017 14:59

electrum suggested changes Jan 23, 2017

View reviewed changes

ArturGajowy mentioned this pull request Jan 24, 2017

Allow multiple violation files, including classpath references. Fixes #22 and #42. gaul/modernizer-maven-plugin#56

Closed

ArturGajowy force-pushed the fix_test_false_postives_testng_543 branch from 472e2d4 to a757afb Compare January 24, 2017 00:07

ArturGajowy assigned electrum Jan 25, 2017

kokosing approved these changes Jan 25, 2017

View reviewed changes

ArturGajowy force-pushed the fix_test_false_postives_testng_543 branch 2 times, most recently from b245e13 to e585e7c Compare January 25, 2017 12:37

ArturGajowy force-pushed the fix_test_false_postives_testng_543 branch from 3e882cf to c2d4ed2 Compare January 26, 2017 18:14

electrum reviewed Feb 11, 2017

View reviewed changes

ArturGajowy force-pushed the fix_test_false_postives_testng_543 branch from c2d4ed2 to 73e70ce Compare February 13, 2017 14:21

ArturGajowy force-pushed the fix_test_false_postives_testng_543 branch from 73e70ce to 8d6556c Compare February 20, 2017 14:20

ArturGajowy added 3 commits March 8, 2017 13:29

Add a shim for TestNG's Assert to handle Iterables correctly

72753d2

Add RowPagesBuilder.getTypesWithoutHash()

84e6079

Fix TestHashJoinOperator affected by TestNG bug

bdbe15a

See testng-team/testng#543.

ArturGajowy added 6 commits March 8, 2017 13:29

Fix AbstractTestQueries tests affected by TestNG bug

623c880

See testng-team/testng#543.

Fix TestCassandraDistributed tests affected by TestNG bug

41f5763

Extract parameterized test methods in AbstractTestQueries to address differences between cassandra and other connectors. See testng-team/testng#543.

Fix TestHiveIntegrationSmokeTest affected by TestNG bug

dcf9f25

See testng-team/testng#543.

Fix TestAggregationOperator affected by TestNG bug

27db779

See testng-team/testng#543.

Fix usages of TestNG's Assert.assertEquals(Iterable, Iterable)

fadc0d2

ArturGajowy force-pushed the fix_test_false_postives_testng_543 branch from 8d6556c to 3367b57 Compare March 8, 2017 12:41

ArturGajowy mentioned this pull request Apr 4, 2017

Hive statistics + POC usage (v7) #7212

Closed

findepi changed the title ~~Fix test false postives due to TestNG 543~~ Fix test false postives due to TestNG's #543 Apr 7, 2017

electrum closed this Apr 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test false postives due to TestNG's #543 #7204

Fix test false postives due to TestNG's #543 #7204

ArturGajowy commented Jan 23, 2017 •

edited

Loading

losipiuk Jan 23, 2017

losipiuk Jan 23, 2017

ArturGajowy Jan 23, 2017

electrum commented Jan 23, 2017

electrum commented Jan 23, 2017

electrum left a comment

ArturGajowy commented Jan 25, 2017

kokosing left a comment

kokosing Jan 25, 2017

ArturGajowy Jan 25, 2017

kokosing Jan 25, 2017

ArturGajowy Jan 25, 2017

kokosing Jan 25, 2017

ArturGajowy Jan 25, 2017

kokosing Jan 25, 2017

ArturGajowy Jan 25, 2017

ArturGajowy commented Jan 25, 2017

ArturGajowy commented Jan 26, 2017

ArturGajowy commented Feb 3, 2017

electrum commented Feb 11, 2017

electrum Feb 11, 2017

electrum Feb 11, 2017

electrum Feb 11, 2017

electrum Feb 11, 2017

ArturGajowy commented Feb 13, 2017

ArturGajowy commented Feb 20, 2017

ArturGajowy commented Feb 28, 2017

ArturGajowy commented Mar 8, 2017

ArturGajowy commented Mar 16, 2017

electrum commented Apr 7, 2017

Fix test false postives due to TestNG's #543 #7204

Fix test false postives due to TestNG's #543 #7204

Conversation

ArturGajowy commented Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum commented Jan 23, 2017

electrum commented Jan 23, 2017

electrum left a comment

Choose a reason for hiding this comment

ArturGajowy commented Jan 25, 2017

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArturGajowy commented Jan 25, 2017

ArturGajowy commented Jan 26, 2017

ArturGajowy commented Feb 3, 2017

electrum commented Feb 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArturGajowy commented Feb 13, 2017

ArturGajowy commented Feb 20, 2017

ArturGajowy commented Feb 28, 2017

ArturGajowy commented Mar 8, 2017

ArturGajowy commented Mar 16, 2017

electrum commented Apr 7, 2017

ArturGajowy commented Jan 23, 2017 •

edited

Loading