Refactor pytest unit tests to dbt unit tests

davidbooke4 commented 3 weeks ago

Description & motivation

The goal of this PR is to move the pytest unit tests to dbt unit tests.

Key Changes

Refactor existing pytest unit tests to dbt unit tests
Add dbt unit tests to stg_ga4__events for url_parsing macros which didn't have pytest unit tests
Add or event_source is null condition to session_ fields in stg_ga4__sessions_traffic_sources model. I did this because it seemed that sessions with a null event_source but non-null other properties were incorrectly being assigned (none) values and attributed to the Direct default_channel_grouping.
Remove pytest unit test files for unit tests that have been moved to dbt unit tests
Add job to run_unit_tests_on_pr.yml for executing the dbt unit tests. To support the Github Action that will execute this job, I've done the following:
- Added additional secrets and environment variables to this repo and declared environment variables in run_unit_tests_on_pr.yml
- Added a profiles.yml file for the Github Action to use to connect to BigQuery
- Added a default profile in dbt_project.yml
- Added conditional logic in base_select.sql and src_ga4.yml so environment variables are used when the source_project and property_ids variables are not set in dbt_project.yml
- Added conditional logic to incremental models to look for incremental days variable from environment variables and then project variables
- Added conditional logic to models that are enabled/disabled based on the declaration of project variables to check for environment variables in addition to project variables
- Added conditional logic to base_ga4__events.sql so it looks for start date variable from both project and environment variables
- Added comments in README.md related to the unit tests in the package
Add conditional logic in base_ga4__events that checks whether the --empty flag was passed in the dbt run/build command. Without this, the model fails due to an error because a limit 0 is added to from {{ source('ga4', 'events') }} and the _table_suffix field is not recognized.

Notes

I've commented out the unit test in stg_ga4__sessions_traffic_sources_last_non_direct_daily.yml due to this bug which is preventing migrating this pytest unit test to a dbt unit test. For the same reason, I've kept the pytest unit test associated with this model.
Evidence of the added run_dbt_unit_tests Github Action can be found here. I had to create a draft PR not from a fork in order to test the changes made in run_unit_tests_on_pr.yml.

Checklist

[x] I have verified that these changes work locally
[x] I have updated the README.md (if applicable)
[x] I have added tests & descriptions to my models (and macros if applicable)
[x] I have run dbt test and python -m pytest . to validate existing tests

adamribaudo-velir commented 3 weeks ago

@davidbooke4 amazing! Just 3 things:

can you remove the unit_tests folder with the legacy pytest stuff if you feel it's no longer necessary?
I'm a little concerned about storing ga4_source_categories.csv in both the seeds folder and the fixtures folder . There's no way to let the unit tests reference it from the seeds folder?
Can you update https://github.com/Velir/dbt-ga4/blob/main/.github/workflows/run_unit_tests_on_pr.yml to run the unit tests rather than pytests?

davidbooke4 commented 3 weeks ago

@adamribaudo-velir I left the unit tests folder since I wasn't able to migrate the sessions_traffic_sources_last_non_direct_daily pytest unit test to a dbt unit test because of this issue that you opened a few months back. Do you think not moving that one test justifies keeping the old unit_tests folder, or are we okay to still remove it and not have a unit test for that model for the time being until a fix is implemented for the bug?

adamribaudo-velir commented 3 weeks ago

@adamribaudo-velir I left the unit tests folder since I wasn't able to migrate the sessions_traffic_sources_last_non_direct_daily pytest unit test to a dbt unit test because of this issue that you opened a few months back. Do you think not moving that one test justifies keeping the old unit_tests folder, or are we okay to still remove it and not have a unit test for that model for the time being until a fix is implemented for the bug?

Oh good call. No harm in leaving it until that bug is resolved. We'll just want to make sure that both this Pytest + the unit tests are running during CI

davidbooke4 commented 2 weeks ago

@adamribaudo-velir I left the unit tests folder since I wasn't able to migrate the sessions_traffic_sources_last_non_direct_daily pytest unit test to a dbt unit test because of this issue that you opened a few months back. Do you think not moving that one test justifies keeping the old unit_tests folder, or are we okay to still remove it and not have a unit test for that model for the time being until a fix is implemented for the bug?

Oh good call. No harm in leaving it until that bug is resolved. We'll just want to make sure that both this Pytest + the unit tests are running during CI

Alright, this is all set for another look whenever you get the chance!

davidbooke4 commented 1 week ago

@adamribaudo-velir I also wanted to call out that these unit tests will run when users install this package and execute a dbt run or dbt build. I was thinking of adding a note in the README about including the --exclude-resource-type unit_test flag in whatever folks are using to orchestrate and execute dbt, but I wanted to get your thoughts on it as well!

dgitis commented 1 week ago

Thank you both for all of your work on this. I've been watching the conversation for awhile.

I just tried running this and it seems that you get a "Parsing Error" if you don't have the stg_ga4__derived _user_properties model enabled.

This model only gets enabled when you have derived_user_properties set.

{{ config(
  enabled = true if var('derived_user_properties', false) or env_var('GA4_DERIVED_USER_PROPERTIES', false) else false,
  materialized = "table"
) }}

I think I should maybe report this as a dbt issue. I've had a number of issues where package YML cause issues when you override at the project level. Introducing this new testing mechanism just makes them worse.

I'm thinking as a workaround, we could maybe create an empty View when the variable is not set rather than set enabled = false.

davidbooke4 commented 1 week ago

Thank you both for all of your work on this. I've been watching the conversation for awhile.

I just tried running this and it seems that you get a "Parsing Error" if you don't have the stg_ga4__derived _user_properties model enabled.

This model only gets enabled when you have derived_user_properties set.
{{ config(
  enabled = true if var('derived_user_properties', false) or env_var('GA4_DERIVED_USER_PROPERTIES', false) else false,
  materialized = "table"
) }}
I think I should maybe report this as a dbt issue. I've had a number of issues where package YML cause issues when you override at the project level. Introducing this new testing mechanism just makes them worse.

I'm thinking as a workaround, we could maybe create an empty View when the variable is not set rather than set enabled = false.

Thanks for looking this over @dgitis!

There's this open issue and PR related to disabling unit tests when their corresponding models are disabled. I was able to compile a project in which I installed this version of the dbt-ga4 package and then set the derived_session_properties, derived_user_properties, and conversion_events variables, so I don't think there's a problem with project variables overriding package variables.

I'm hopeful that the PR will be resolved soon and this problem will be fixed, but materializing the model as an empty view is an alternative I hadn't considered. I was debating keeping the pytest tests in place for these few models that are enabled based on project variables and then moving them to dbt unit tests once that issue is resolved.

Velir / dbt-ga4