Skip to content
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.

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

Initial attempt to get spark working with new testing framework #299

Merged
merged 4 commits into from
Mar 29, 2022

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Mar 14, 2022

resolves #298

Description

Initial implementation of new testing framework in Spark

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt-spark next" section.

@cla-bot cla-bot bot added the cla:yes label Mar 14, 2022
@jtcohen6
Copy link
Contributor

jtcohen6 commented Mar 14, 2022

Nice work! Just in case you didn't see, the currently failing test is because spark-thrift is running Spark v2, which doesn't support CTEs nested inside subqueries:

# the local cluster currently tests on spark 2.x, which does not support this
# if we upgrade it to 3.x, we can enable this test
# test_dbt_ephemeral_data_tests: data_test_ephemeral_models

The other connection methods (Databricks) are running Spark v3, which does

@gshank
Copy link
Contributor Author

gshank commented Mar 15, 2022

The spark thrift one was the one I tested locally, and that test passed :). So I'm guessing locally we're using a different Spark version? I'll mark that test as a skip though.

@gshank gshank force-pushed the ct-360-new_test_framework_spark branch from ab54007 to ca07af1 Compare March 15, 2022 16:33
@gshank gshank force-pushed the ct-360-new_test_framework_spark branch from ca07af1 to a00272e Compare March 21, 2022 20:24
@McKnight-42
Copy link
Contributor

McKnight-42 commented Mar 23, 2022

Hi Gerda the test you have failing for odbc cluster may be due to not having this change implemented #301. at least thats my best guess off the error thats being thrown.

Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -4,8 +4,8 @@
from pathlib import Path


TestResults = namedtuple(
'TestResults',
ResultHolder = namedtuple(
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha I actually suggested Matt to convert this to the same name

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 saw that and used the same name :). Pytest was also trying to collect "TestResults" as a test class too.

@@ -77,6 +77,8 @@ def __init__(self):


class TestArgs:
__test__ = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this line fix the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pytest test collection gathers up all classes that start with 'Test', so the test = False marks this class as "not a test".

@gshank gshank merged commit bbff5c7 into main Mar 29, 2022
@gshank gshank deleted the ct-360-new_test_framework_spark branch March 29, 2022 21:16
ueshin added a commit to databricks/dbt-databricks that referenced this pull request Apr 1, 2022
### Description

Port testing framework changes from dbt-labs/dbt-spark#299 and dbt-labs/dbt-spark#314.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-360] Implement basic tests in new testing framework in the Spark plugin
4 participants