Dumps the ported typer tests and adds appropriate testing mechanisms. #1574

johnedquinn · 2024-09-03T18:44:28Z

Description

Dumps the ported typer tests and adds appropriate testing mechanisms.

Structure

The testing structure is as follows:
- Tests reside in resources under tests
- Tests are serialized as Ion structs and have three mandatory fields: name, type, and body. The type is used to request the TestBuilderFactory which TestBuilder to use. The name, in combination with the path to the test file, is used to create a TestId. The body (a struct) is passed to the TestBuilder to allow for user-defined tests.
- The newly introduced TyperTests just invokes the TestProvider to deserialize the tests and shuttle them through JUnit.
This should provide adequate flexibility for executing varying test types.

Other Information

Updated Unreleased Section in CHANGELOG: NO
Any backward-incompatible changes? NO
Any new external dependencies? NO
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? YES

License Information

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

johnedquinn · 2024-09-03T18:46:44Z

partiql-planner/src/test/kotlin/org/partiql/planner/internal/typer/PlanTyperTestsPorted.kt

These slight changes made it easier to serialize the tests.

johnedquinn · 2024-09-03T18:49:21Z

partiql-planner/src/test/kotlin/org/partiql/planner/internal/typer/TyperTests.kt

+import org.partiql.planner.test.TestProviderBuilder
+import java.util.stream.Stream
+
+class TyperTests {


This is the replacement for PlanTyperTestsPorted.

Is there a way to confirm all of the existing tests are included in TyperTests?

johnedquinn · 2024-09-03T18:50:36Z

partiql-planner/src/testFixtures/resources/tests/aggregations.ion

These were unused.

alancai98

Nice job w/ porting the tests! Most comments/questions were about some missing context and possible simplifications

alancai98 · 2024-09-05T22:24:00Z

partiql-planner/src/test/kotlin/org/partiql/planner/internal/typer/TyperTests.kt

+import org.partiql.planner.test.TestProviderBuilder
+import java.util.stream.Stream
+
+class TyperTests {


Is there a way to confirm all of the existing tests are included in TyperTests?

alancai98 · 2024-09-06T19:47:25Z

partiql-planner/src/test/kotlin/org/partiql/planner/internal/typer/PrintPlanTyperTestsPorted.kt

+    fun print() {
+        val other = PlanTyperTestsPorted.TestProvider().provideArguments(null).map { it.get()[0] as PlanTyperTestsPorted.TestCase }.toList()
+        val testCases =
+            other + PlanTyperTestsPorted.collections() + PlanTyperTestsPorted.decimalCastCases() + PlanTyperTestsPorted.selectStar() + PlanTyperTestsPorted.sessionVariables() + PlanTyperTestsPorted.bitwiseAnd() + PlanTyperTestsPorted.unpivotCases() + PlanTyperTestsPorted.joinCases() + PlanTyperTestsPorted.excludeCases() + PlanTyperTestsPorted.orderByCases() + PlanTyperTestsPorted.tupleUnionCases() + PlanTyperTestsPorted.aggregationCases() + PlanTyperTestsPorted.scalarFunctions() + PlanTyperTestsPorted.distinctClauseCases() + PlanTyperTestsPorted.pathExpressions() + PlanTyperTestsPorted.caseWhens() + PlanTyperTestsPorted.nullIf() + PlanTyperTestsPorted.coalesce() + PlanTyperTestsPorted.subqueryCases() + PlanTyperTestsPorted.dynamicCalls() + PlanTyperTestsPorted.scanCases() + PlanTyperTestsPorted.pivotCases() + PlanTyperTestsPorted.isTypeCases() + PlanTyperTestsPorted.castCases()


nit: this currently spans one line. could break it up for better readability

alancai98 · 2024-09-06T20:42:16Z

partiql-planner/src/testFixtures/kotlin/org/partiql/planner/test/Test.kt

+interface Test {
+
+    /**
+     * @return the name of the test.
+     */
+    fun getName(): String
+
+    /**
+     * Should throw an exception if a failure occurs.
+     */
+    fun assert()
+}


Do we foresee future Test implementations other than TyperTest? I feel like it's slight overkill to have a broader Test interface.

Yes. The reason we'd like to do this is to create a starting point by which we may serialize all of our tests in a uniform manner. For execution, typing, etc.

alancai98 · 2024-09-06T20:46:56Z

partiql-planner/src/testFixtures/kotlin/org/partiql/planner/test/TestBuilder.kt

+ * @see StructElement
+ * @see PartiQLTestProvider
+ */
+interface TestBuilder {


Similarly for the TestBuilder and TestBuilderFactory, seems like a lot of additional boilerplate unless we foresee adding more than the typing tests within partiql-planner?

alancai98 · 2024-09-06T21:05:38Z

partiql-planner/src/testFixtures/resources/tests/plan-typer/plan-typer-tests-ported.ion

+      cwd:[
+      ]


The vast majority of the typing tests use an empty session directory list. Wonder if we could simplify the logic by making a non-specified cwd equivalent to an empty session directory list.

I like this! I'll update.

alancai98 · 2024-09-06T21:12:11Z

partiql-planner/src/testFixtures/kotlin/org/partiql/planner/test/PartiQLTestProvider.kt

@@ -23,6 +23,7 @@ import kotlin.io.path.toPath
 * The PartiQLTestProvider is a simple utility for indexing SQL statements within files for re-use across library tests.
 */
 class PartiQLTestProvider {
+    // TODO: Rename this to StatementProvider to better reflect its purpose


Could address in this PR?

alancai98 · 2024-09-06T21:18:35Z

partiql-planner/src/testFixtures/kotlin/org/partiql/planner/test/TestId.kt

+    private fun getQualifier(): Array<String> {
+        return _qualifier
+    }
+
+    private fun getName(): String {
+        return _name
+    }


Are these getters necessary? Since this is just in the tests, this indirection is not needed. I feel like it would be much simpler to just directly reference the _qualifer and _name.

alancai98 · 2024-09-06T21:20:37Z

partiql-planner/src/testFixtures/resources/tests/plan-typer/plan-typer-tests-ported.ion

@@ -0,0 +1,6717 @@
+
+test::{


At some point could be helpful to break into different files and/or namespaces like the conformance tests currently do. We could track in some TODO/issue.

alancai98 · 2024-09-06T21:24:57Z

partiql-planner/src/test/kotlin/org/partiql/planner/TyperTest.kt

+        }
+    }
+
+    /**


Could provide the description of the test format in some other more visible location such as https://github.com/johnedquinn/partiql-lang-jvm/blob/v1-tests-dump/partiql-planner/src/testFixtures/resources/README.adoc.

Something like https://github.com/partiql/partiql-tests/blob/main/docs/partiql-tests-schema-proposal.md#evaluation-tests, could make it easier to add future tests.

alancai98 · 2024-09-06T21:32:49Z

partiql-planner/src/test/kotlin/org/partiql/planner/TyperTest.kt

@@ -0,0 +1,236 @@
+package org.partiql.planner


Had some broader questions for porting the typing tests since I may be missing some context.

Is the goal here to eventually delete the PlanTyperTestsPorted (and related files)?

Where will the ported typing tests eventually live? In the conformance test suite?

At what point will we delete the existing PlanTyperTestsPorted (and related files)?

For future plan typing tests we want to add, what's the process for adding a test? Just add the test to the plan-typer-tests-ported.ion file?

Yes.

I'd personally like to extract these interfaces into another package: test/partiql-test-interfaces that can be imported as a test dependency across the project. Any particular subproject may have local serialized tests -- and once the team agrees upon the tests' correctness, they can be moved into the conformance test suite. If you'd like, I can move these interfaces immediately in this PR.

The only open action item for PlanTyperTestsPorted has to do with the serialization of errors, which is being worked on. I actually don't need to keep it right now. I can always resurface the PrintPlanTyperTestsPorted from an older commit to re-serialize when the error serialization is addressed.

You can add a test to the tests directory in any file you'd like.

Dumps the ported typer tests and adds appropriate testing mechanisms.

a4cd8a2

johnedquinn commented Sep 3, 2024

View reviewed changes

johnedquinn marked this pull request as ready for review September 3, 2024 18:59

johnedquinn requested a review from alancai98 September 5, 2024 18:16

alancai98 reviewed Sep 6, 2024

View reviewed changes

johnedquinn marked this pull request as draft September 19, 2024 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dumps the ported typer tests and adds appropriate testing mechanisms. #1574

Dumps the ported typer tests and adds appropriate testing mechanisms. #1574

johnedquinn commented Sep 3, 2024 •

edited

Loading

johnedquinn Sep 3, 2024

johnedquinn Sep 3, 2024

alancai98 Sep 5, 2024

johnedquinn Sep 3, 2024

alancai98 left a comment

alancai98 Sep 5, 2024

alancai98 Sep 6, 2024

alancai98 Sep 6, 2024

johnedquinn Sep 9, 2024

alancai98 Sep 6, 2024

alancai98 Sep 6, 2024

johnedquinn Sep 9, 2024

alancai98 Sep 6, 2024

alancai98 Sep 6, 2024

alancai98 Sep 6, 2024

alancai98 Sep 6, 2024

alancai98 Sep 6, 2024

johnedquinn Sep 9, 2024

+                      }
+                  }
+                  /**

Dumps the ported typer tests and adds appropriate testing mechanisms. #1574

Are you sure you want to change the base?

Dumps the ported typer tests and adds appropriate testing mechanisms. #1574

Conversation

johnedquinn commented Sep 3, 2024 • edited Loading

Description

Structure

Other Information

License Information

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

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

johnedquinn commented Sep 3, 2024 •

edited

Loading