Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 6, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8fc169
gradle: fix test target for cli, examples, pts
evanj Dec 24, 2020
0ca86ac
code review fixes
evanj Jan 5, 2021
89211ee
restore incorrectly removed runtime dependency
evanj Jan 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions cli/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
plugins {
id 'org.jetbrains.kotlin.jvm'
id 'io.gitlab.arturbosch.detekt'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this plugin removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, done, thanks.

id 'application'
}

Expand Down Expand Up @@ -35,10 +34,7 @@ dependencies {
implementation 'net.sf.jopt-simple:jopt-simple:[5.0,6.0)'
implementation 'org.jetbrains.kotlin:kotlin-stdlib-jdk8'

testImplementation 'org.jetbrains.kotlin:kotlin-test'
testImplementation 'pl.pragmatists:JUnitParams:[1.0.0,1.1.0)'

testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.0'
testImplementation 'org.junit.vintage:junit-vintage-engine:5.7.0'
}

jacocoTestReport {
Expand Down
9 changes: 9 additions & 0 deletions cli/test/org/partiql/cli/CliTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,4 +112,13 @@ class CliTest {

assertEquals("<<{'a': 1}>>", actual)
}

@Test
fun withPartiQLPrettyOutput() {
val subject = makeCli("SELECT * FROM input_data", "{a: 1, b: 2}",
outputFormat = OutputFormat.PARTIQL_PRETTY)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

val actual = subject.runAndOutput()

assertEquals("<<\n {\n 'a': 1,\n 'b': 2\n }\n>>", actual)
}
}
7 changes: 2 additions & 5 deletions examples/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,11 @@ application {

dependencies {
implementation project(":lang")

implementation 'org.jetbrains.kotlin:kotlin-stdlib-jdk8'
implementation 'com.amazonaws:aws-java-sdk-s3:1.11.554'
implementation 'com.amazonaws:aws-java-sdk-s3control:1.11.554'

implementation 'org.jetbrains.kotlin:kotlin-test'
implementation 'junit:junit:4.12'

testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.0'
testImplementation 'org.junit.vintage:junit-vintage-engine:5.7.0'
}

jacocoTestReport {
Expand Down
2 changes: 0 additions & 2 deletions examples/src/kotlin/org/partiql/examples/CsvExprValueExample.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package org.partiql.examples

import org.junit.*
import org.junit.Assert.*
import com.amazon.ion.*
import com.amazon.ion.system.*
import org.partiql.examples.util.Example
Expand Down
2 changes: 0 additions & 2 deletions examples/src/kotlin/org/partiql/examples/CustomFunctionsExample.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
package org.partiql.examples

import org.junit.*
import com.amazon.ion.system.*
import org.partiql.examples.util.Example
import org.partiql.lang.*
import org.partiql.lang.eval.*
import java.io.PrintStream
import kotlin.test.*

/** A simple fibonacci calculator. */
private fun calcFib(n: Long): Long = when (n) {
Expand Down
4 changes: 1 addition & 3 deletions examples/src/kotlin/org/partiql/examples/ParserErrorExample.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package org.partiql.examples

import org.junit.*
import kotlin.test.*
import com.amazon.ion.*
import com.amazon.ion.system.*
import org.partiql.examples.util.Example
Expand All @@ -28,7 +26,7 @@ class ParserErrorExample(out: PrintStream) : Example(out) {
print("Invalid PartiQL query:", invalidQuery)
parser.parseExprNode(invalidQuery)

fail("ParserException was not thrown")
throw Exception("ParserException was not thrown")
} catch (e: ParserException) {
val errorContext = e.errorContext!!

Expand Down
10 changes: 7 additions & 3 deletions examples/src/kotlin/org/partiql/examples/ParserExample.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
package org.partiql.examples

import kotlin.test.*
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: unused import

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Demonstrates the use of [SqlParser] and [PartiqlAst].
Expand Down Expand Up @@ -45,7 +45,9 @@ class ParserExample(out: PrintStream) : Example(out) {
// version of the s-expression form to an instance of [ExprNode].
val deserializedAst = serializedAst.toExprNode(ion)
// Verify that we have the correct AST.
assertEquals(originalAst, deserializedAst)
if (originalAst != deserializedAst) {
throw Exception("expected AST to be equal")
}
Comment on lines +47 to +49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Here we show how to parse a query directly to a PartiqlAst statement
val statement = parser.parseAstStatement(query)
Expand All @@ -55,6 +57,8 @@ class ParserExample(out: PrintStream) : Example(out) {
// and back into a PartiqlAst statement
val roundTrippedStatement = PartiqlAst.transform(elements) as PartiqlAst.Statement
// Verify that we have the original Partiql Ast statement
assertEquals(statement, roundTrippedStatement)
if (statement != roundTrippedStatement) {
throw Exception("Expected statements to be the same")
}
Comment on lines +59 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same argument here concerning assertEquals

}
}
5 changes: 0 additions & 5 deletions lang/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,12 @@ dependencies {
api 'org.partiql:partiql-ir-generator-runtime:0.3.0'

// test-time dependencies
testImplementation 'org.jetbrains.kotlin:kotlin-reflect'
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'
Copy link
Member

@alancai98 alancai98 Jan 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

testImplementation 'org.junit.jupiter:junit-jupiter-api:5.7.0'
testImplementation 'org.junit.vintage:junit-vintage-engine:5.7.0'
testImplementation 'org.junit.jupiter:junit-jupiter-params:5.7.0'
// PTS
testImplementation project(':testscript')
Comment on lines -60 to -61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

task generatePigDomains {
Expand Down
7 changes: 1 addition & 6 deletions pts/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,9 @@ dependencies {
implementation 'org.jetbrains.kotlin:kotlin-stdlib-jdk8'

// test-time dependencies
testImplementation 'org.jetbrains.kotlin:kotlin-reflect'
testImplementation 'org.jetbrains.kotlin:kotlin-test'
testImplementation 'pl.pragmatists:JUnitParams:[1.0.0,1.1.0)'
testImplementation 'org.assertj:assertj-core:[3.11.0,3.12.0)'
testImplementation project(':lang')
testImplementation project(':testscript')

testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.0'
testImplementation 'org.junit.vintage:junit-vintage-engine:5.7.0'
}

jacocoTestReport {
Expand Down