Skip to content

Chore: Reorganize engine adapter integration tests#3188

Merged
erindru merged 2 commits intomainfrom
erin/refactor-integration-tests
Sep 26, 2024
Merged

Chore: Reorganize engine adapter integration tests#3188
erindru merged 2 commits intomainfrom
erin/refactor-integration-tests

Conversation

@erindru
Copy link
Collaborator

@erindru erindru commented Sep 26, 2024

Currently, in order to write an integration test against one of the engines, it needs to be tacked on to test_integration.py.

This file is starting to get a bit unwieldy:

  • It's ~2700 lines and growing
  • Every test using TestContext automatically gets 3 versions: df, query and pyspark. Often you only want one of these, typically query (and almost never pyspark) leading to a bunch of skipping logic within tests if the test_type is not query
  • Every new test has to go into this file, even if its only for a specific database, which means you have to add skipping logic to ensure its being invoked for the correct database
  • It sits at the same level in the hierarchy as the unit tests for each engine adapter, making it slightly confusing as to where new tests should go

This PR makes the following changes:

  • Moves everything related to the engine adapter integration tests into an integration subfolder
  • Moves the engine adapter / TestContext setup logic into a common place
  • Demonstrates adding an integration test file scoped to a specific engine (to be extended on in future PR's)

The goal here is to set the foundation for breaking these up further in future. Tests affecting all adapters can stay in test_integration.py and then tests for a specific database can be put in test_integration_<database>.py.

The immediate motivation was needing somewhere to put tests for the upcoming partition management logic in the Athena and Postgres adapters but these changes also give the option of moving eg the pyspark tests out of the main file and into a Spark-specific area

@erindru erindru force-pushed the erin/refactor-integration-tests branch from 1bdd5f7 to 5944034 Compare September 26, 2024 01:47
@izeigerman izeigerman requested a review from eakmanrq September 26, 2024 15:07
#filters:
# branches:
# only:
# - main
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I needed to show that all the tests were still running / passing (if there was an issue with these, it would only show up post-merge)

I'll uncomment this prior to merge

Copy link
Collaborator

@eakmanrq eakmanrq left a comment

Choose a reason for hiding this comment

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

Thanks for laying the foundation for improving this!

@erindru erindru force-pushed the erin/refactor-integration-tests branch from df5f043 to 244ce70 Compare September 26, 2024 20:57
@erindru erindru merged commit df4e188 into main Sep 26, 2024
@erindru erindru deleted the erin/refactor-integration-tests branch September 26, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants