gradle: fix test target for cli, examples, pts #351

evanj · 2020-12-24T20:27:03Z

Description of changes:

The following gradle targets were not running any of the unit tests
contained in their directories due to incorrect test dependencies. It
appears these projects all use JUnit 5 "vintage" tests, so need that
engine as a test dependency.

:cli:test
:examples:test
:pts:test

CliTest: Add test for the PARTIQL_PRETTY output format added in commit
9175002. This was how I discovered this issue.

examples: Remove tests from the implementation dependencies.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The following gradle targets were not running any of the unit tests contained in their directories due to incorrect test dependencies. It appears these projects all use JUnit 5 "vintage" tests, so need that engine as a test dependency. :cli:test :examples:test :pts:test CliTest: Add test for the PARTIQL_PRETTY output format added in commit 9175002. This was how I discovered this issue. examples: Remove tests from the implementation dependencies.

therapon · 2020-12-24T21:55:34Z

examples/src/kotlin/org/partiql/examples/ParserExample.kt

+        if (originalAst != deserializedAst) {
+            throw Exception("expected AST to be equal")
+        }


Could we instead use the assertEquals with a message parameter for the exception? I am not sure what we are gaining by using if and throwing our own exception here, perhaps I am missing something though.

The reason was to avoid requiring the kotlin-test JAR from code that is technically not an automated unit test. I will revert that part of the change since you would prefer to use this dependency.

Thanks for explaining. In that case, ignore my comments for this file on using assert. Let's keep the examples clean of testing libs.

therapon · 2020-12-24T21:55:52Z

examples/src/kotlin/org/partiql/examples/ParserExample.kt

+        if (statement != roundTrippedStatement) {
+            throw Exception("Expected statements to be the same")
+        }


Same argument here concerning assertEquals

therapon · 2020-12-24T21:57:03Z

lang/build.gradle

-    // PTS
-    testImplementation project(':testscript')


Was this dependency breaking anything? This (I believe) is what hooks into the PTS (included in this repo) system which runs more tests focusing on spec compliance.

It was not breaking anything, but it seems to be unnecessary as the code builds without it? I haven't read the code in detail, but it appears to me that the dependency tree is that the pts targets/directory imports the code in testscript. I was assuming that means those are the targets that actually use this code. Nothing in the lang directory references testscript AFAIK?

I barely understand this though; I just was curious if all these dependencies were actually needed. I will revert my changes to this file: the important updates are to cli, examples, and pts so ./gradlew test actually executes the tests in those directories.

alancai98

Good catch. I think my previous PR (#338) to fix Gradle to run on ParameterizedTests introduced this testing target bug.

Only have a few nits/questions.

alancai98 · 2020-12-24T21:57:10Z

examples/src/kotlin/org/partiql/examples/ParserExample.kt

 import com.amazon.ion.system.*
 import org.partiql.examples.util.Example
 import org.partiql.lang.ast.*
 import org.partiql.lang.domains.PartiqlAst
 import org.partiql.lang.syntax.*
 import java.io.PrintStream
+import java.util.Objects


nit: unused import

Oops thanks sorry; This was a stray import from a previous edit.

alancai98 · 2020-12-24T22:00:49Z

cli/test/org/partiql/cli/CliTest.kt

+        val subject = makeCli("SELECT * FROM input_data", "{a: 1, b: 2}",
+            outputFormat = OutputFormat.PARTIQL_PRETTY)


nit: could move onto the same line as the previous test

Suggested change

val subject = makeCli("SELECT * FROM input_data", "{a: 1, b: 2}",

outputFormat = OutputFormat.PARTIQL_PRETTY)

val subject = makeCli("SELECT * FROM input_data", "{a: 1, b: 2}", outputFormat = OutputFormat.PARTIQL_PRETTY)

alancai98 · 2020-12-24T22:01:33Z

cli/build.gradle

@@ -1,6 +1,5 @@
 plugins {
    id 'org.jetbrains.kotlin.jvm'
-    id 'io.gitlab.arturbosch.detekt'


Why is this plugin removed?

I don't understand gradle. I assumed since this file has no references to it, this is unnecessary?

The top level build.gradle references detekt, and I assumed the apply plugin thing attempts to apply the plugin to all subprojects? If so, doesn't that mean this is unnecessary, since the top level project includes it?

FWIW, with this change, the ./gradlew cli:detekt task still seems to work?

~/partiql-lang-kotlin $ ./gradlew cli:detekt --info ... > Task :cli:detekt UP-TO-DATE ...

I also don't understand gradle too well. You seem to be right regarding the top level references to detekt, which is applied to all the subprojects. Since the detekt plugin isn't referenced in the other subprojects, could you also remove the detekt plugin from

lang/build.gradle

pts/build.gradle

testscript/build.gradle

Okay, done, thanks.

evanj

I will make the suggested changes ... but maybe not for a few days; thanks for the prompt review!

evanj · 2020-12-24T23:19:02Z

examples/src/kotlin/org/partiql/examples/ParserExample.kt

+        if (originalAst != deserializedAst) {
+            throw Exception("expected AST to be equal")
+        }


The reason was to avoid requiring the kotlin-test JAR from code that is technically not an automated unit test. I will revert that part of the change since you would prefer to use this dependency.

evanj · 2020-12-24T23:23:27Z

lang/build.gradle

-    // PTS
-    testImplementation project(':testscript')


It was not breaking anything, but it seems to be unnecessary as the code builds without it? I haven't read the code in detail, but it appears to me that the dependency tree is that the pts targets/directory imports the code in testscript. I was assuming that means those are the targets that actually use this code. Nothing in the lang directory references testscript AFAIK?

I barely understand this though; I just was curious if all these dependencies were actually needed. I will revert my changes to this file: the important updates are to cli, examples, and pts so ./gradlew test actually executes the tests in those directories.

evanj · 2020-12-24T23:23:50Z

examples/src/kotlin/org/partiql/examples/ParserExample.kt

 import com.amazon.ion.system.*
 import org.partiql.examples.util.Example
 import org.partiql.lang.ast.*
 import org.partiql.lang.domains.PartiqlAst
 import org.partiql.lang.syntax.*
 import java.io.PrintStream
+import java.util.Objects


Oops thanks sorry; This was a stray import from a previous edit.

evanj · 2020-12-24T23:28:01Z

cli/build.gradle

@@ -1,6 +1,5 @@
 plugins {
    id 'org.jetbrains.kotlin.jvm'
-    id 'io.gitlab.arturbosch.detekt'


I don't understand gradle. I assumed since this file has no references to it, this is unnecessary?

The top level build.gradle references detekt, and I assumed the apply plugin thing attempts to apply the plugin to all subprojects? If so, doesn't that mean this is unnecessary, since the top level project includes it?

FWIW, with this change, the ./gradlew cli:detekt task still seems to work?

~/partiql-lang-kotlin $ ./gradlew cli:detekt --info ... > Task :cli:detekt UP-TO-DATE ...

CliTest.kt: format code on one line ParserExample.kt: remove unnecessary import lang,pts,testscript/build.gradle: remove detekt plugin (not needed)

evanj · 2021-01-05T16:00:10Z

sorry for the delay. I believe I addressed all the comments. Thanks again for the prompt review!

alancai98

Sorry, I missed a deleted line in the first review. Once that's added back, should be approved and merged. Thanks again for the contribution!

alancai98 · 2021-01-06T01:18:21Z

lang/build.gradle

    testImplementation 'org.jetbrains.kotlin:kotlin-test-junit5'
    testImplementation 'pl.pragmatists:JUnitParams:[1.0.0,1.1.0)'
    testImplementation 'org.assertj:assertj-core:[3.11.0,3.12.0)'

-    testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.0'


We need this dependency to run junit ParameterizedTests with ArgumentsSource that we are starting to use (e.g. EvaluatingCompilerFromLetTests.kt). Without this dependency, these ParameterizedTests aren't being run by gradle.

Oops nice catch thanks! This is probably why I should not make wholesale changes without understanding what I am actually doing :)

therapon reviewed Dec 24, 2020

View reviewed changes

alancai98 reviewed Dec 24, 2020

View reviewed changes

evanj commented Dec 24, 2020

View reviewed changes

code review fixes

0ca86ac

CliTest.kt: format code on one line ParserExample.kt: remove unnecessary import lang,pts,testscript/build.gradle: remove detekt plugin (not needed)

alancai98 reviewed Jan 6, 2021

View reviewed changes

restore incorrectly removed runtime dependency

89211ee

alancai98 approved these changes Jan 6, 2021

View reviewed changes

alancai98 merged commit 2ca7702 into partiql:master Jan 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gradle: fix test target for cli, examples, pts #351

gradle: fix test target for cli, examples, pts #351

evanj commented Dec 24, 2020 •

edited

Loading

therapon Dec 24, 2020

evanj Dec 24, 2020

therapon Dec 28, 2020

therapon Dec 24, 2020

therapon Dec 24, 2020

evanj Dec 24, 2020

alancai98 left a comment

alancai98 Dec 24, 2020

evanj Dec 24, 2020

alancai98 Dec 24, 2020

alancai98 Dec 24, 2020

evanj Dec 24, 2020

alancai98 Jan 4, 2021

evanj Jan 5, 2021

evanj left a comment

evanj Dec 24, 2020

evanj Dec 24, 2020

evanj Dec 24, 2020

evanj Dec 24, 2020

evanj commented Jan 5, 2021

alancai98 left a comment

alancai98 Jan 6, 2021 •

edited

Loading

evanj Jan 6, 2021

		val subject = makeCli("SELECT * FROM input_data", "{a: 1, b: 2}",
		outputFormat = OutputFormat.PARTIQL_PRETTY)

gradle: fix test target for cli, examples, pts #351

gradle: fix test target for cli, examples, pts #351

Conversation

evanj commented Dec 24, 2020 • edited Loading

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

alancai98 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

evanj 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

evanj commented Jan 5, 2021

alancai98 left a comment

Choose a reason for hiding this comment

alancai98 Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanj commented Dec 24, 2020 •

edited

Loading

alancai98 Jan 6, 2021 •

edited

Loading