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

Make asset checks matter to integration tests #3687

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

jdangerx
Copy link
Member

@jdangerx jdangerx commented Jun 21, 2024

Overview

Closes #3705 .

What did you change?

  • we weren't running asset checks in our ETL when running in integration tests. Instead of making a new, different ETL job for integration tests, we now just run the same job we use everywhere else, but with whatever dataset configuration we have defined for the tests.
  • we also weren't asserting that the ETL execution actually succeeded. Asset check failures currently cause Exceptions, so we don't technically need this, but it seems prudent to assert success anyways.

Testing

  • verified that the fgd_equipment_null_check asset check passes, by running the raw_eia860 assets and then the _core_eia860__fgd_equipment asset in Dagster
  • ran integration tests with make pytest-coverage and saw the integration tests pass
  • break the fgd_equipment_null_check by making it return AssetCheckResult(passed=False)
  • re-run the _core_eia860__fgd_equipment asset in Dagster and see the check fail
  • re-run integration tests with make pytest-coverage and see that the integration test fails with a bunch of errors about dagster._core.errors.DagsterAssetCheckFailedError: Blocking check 'fgd_equipment_null_c heck' for asset '_core_eia860__fgd_e...

To-do list

Preview Give feedback

@jdangerx
Copy link
Member Author

Frustratingly, running make pytest-coverage on this branch has caused a bevy of apparently unrelated errors in the integration tests so I will need to poke around more to validate that this indeed causes our tests to fail on failed asset checks.

@jdangerx jdangerx force-pushed the fix-failing-asset-checks-not-failing-etl branch from 8dd44ac to bb86c06 Compare June 28, 2024 21:45
Also, don't continue integration test if the ETL run fails.
@jdangerx jdangerx force-pushed the fix-failing-asset-checks-not-failing-etl branch from bb86c06 to f84467d Compare July 2, 2024 16:08
@jdangerx jdangerx changed the title WIP - fail a bunch of asset checks Make asset checks matter to integration tests Jul 2, 2024
@jdangerx jdangerx marked this pull request as ready for review July 2, 2024 16:15
@@ -23,42 +19,6 @@
logger = pudl.logging_helpers.get_logger(__name__)


def pudl_etl_job_factory(
Copy link
Member Author

Choose a reason for hiding this comment

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

This was only being used in conftest so I moved it over to avoid some import issues.

@@ -272,6 +279,29 @@ def ferc1_xbrl_taxonomy_metadata(ferc1_engine_xbrl: sa.Engine):
return result.output_for_node("raw_ferc1_xbrl__metadata_json")


def _pudl_etl_job_factory(
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed one layer of function nesting here because it seemed unnecessary based on the reconstructable jobs docs.

Since we're only using execute_in_process right now anyways, we don't even need a reconstructable job, but it seems fine to leave this because the logic is pretty straightforward now.

The job definition to be executed.
"""
pudl.logging_helpers.configure_root_logger(logfile=logfile, loglevel=loglevel)
if not process_epacems:
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of making whole new JobDefinitions I just return the ones we actually use in other builds - that should make test behavior hew more closely to production behavior.

@jdangerx jdangerx requested review from a team and e-belfer and removed request for a team July 2, 2024 16:19
@jdangerx jdangerx enabled auto-merge July 2, 2024 16:19
@zaneselvans zaneselvans added testing Writing tests, creating test data, automating testing, etc. dagster Issues related to our use of the Dagster orchestrator labels Jul 2, 2024
@jdangerx jdangerx added this pull request to the merge queue Jul 2, 2024
Merged via the queue into main with commit 7c79116 Jul 2, 2024
12 checks passed
@jdangerx jdangerx deleted the fix-failing-asset-checks-not-failing-etl branch July 2, 2024 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dagster Issues related to our use of the Dagster orchestrator testing Writing tests, creating test data, automating testing, etc.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Asset check failures do not fail integration tests
2 participants