-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: Add integration tests for dbt import #5899
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
base: master
Are you sure you want to change the base?
feat: Add integration tests for dbt import #5899
Conversation
Co-authored-by: franciscojavierarceo <4163062+franciscojavierarceo@users.noreply.github.com>
Co-authored-by: franciscojavierarceo <4163062+franciscojavierarceo@users.noreply.github.com>
Co-authored-by: franciscojavierarceo <4163062+franciscojavierarceo@users.noreply.github.com>
Co-authored-by: franciscojavierarceo <4163062+franciscojavierarceo@users.noreply.github.com>
Co-authored-by: franciscojavierarceo <4163062+franciscojavierarceo@users.noreply.github.com>
Co-authored-by: franciscojavierarceo <4163062+franciscojavierarceo@users.noreply.github.com>
…generate it Co-authored-by: franciscojavierarceo <4163062+franciscojavierarceo@users.noreply.github.com>
Bug fixes: - Fix entity.join_keys -> entity.join_key (singular string attribute) - Fix object count assertion (12 -> 9) - Fix entity comparison to use .name (feature_view.entities returns strings) New test coverage: - Error handling: manifest not found, invalid JSON - Type mapping edge cases: Bool, Array, unknown types, parameterized types - Snowflake NUMBER with precision/scale handling - created_timestamp_column parameter - Custom timestamp field override - Generated code validation with ast.parse() Add pytest.ini for test isolation from parent conftest.py Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
- Remove 'dbt test' from workflow (dbt build already runs tests) - Remove unused pytest import from conftest.py - Tables are in-memory and disappear after dbt build completes Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
4423c32 to
37224b0
Compare
The -c flag expects a directory path, not a config file name. Since we're already in the feast_repo directory, feast will automatically find the feature_store.yaml file. Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
Update tests to be flexible with actual dbt output instead of hardcoded values: - Don't assert specific dbt_version - just verify it's present - Use model.database and model.schema dynamically (varies by adapter) - Verify full_table_name is built correctly from components - Fix unused parameter warning in test_create_entity Signed-off-by: David Soria Parra <dsp@php.net> Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
Tags in dbt must be placed inside a config block to appear in the generated manifest.json. Without this, the tags were not being included when dbt build runs in CI. Signed-off-by: David Soria Parra <dsp@php.net> Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
- Remove trailing whitespace from blank lines - Add noqa: E402 comments for imports after pytest.importorskip Signed-off-by: David Soria Parra <dsp@php.net> Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
Signed-off-by: David Soria Parra <dsp@php.net> Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
Add pytest collection hook to automatically skip dbt tests when the manifest.json file hasn't been generated yet. This prevents failures in the unit-test-python workflow which doesn't run dbt build. The dbt-integration-test workflow generates the manifest before running tests, so tests will still execute there. Signed-off-by: David Soria Parra <dsp@php.net> Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
| dbt deps | ||
| dbt build | ||
|
|
||
| - name: Setup Feast project for dbt import test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Can you instead put most of the logic into the Makefile so that we can easily run make dbt-integration-test and swap that here?
|
|
||
| filterwarnings = | ||
| error::_pytest.warning_types.PytestConfigWarning | ||
| error::_pytest.warning_types.PytestUnhandledCoroutineWarning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why remove this?
1. Move dbt integration test logic into Makefile - Add test-python-integration-dbt target to consolidate test steps - Makes it easy to run `make test-python-integration-dbt` locally - Simplifies GitHub Actions workflow 2. Restore pytest warning filter - Re-add error::_pytest.warning_types.PytestUnhandledCoroutineWarning - This was accidentally removed and should be kept Signed-off-by: David Soria Parra <dsp@php.net> Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
YassinNouh21
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed review comments in the latest commit:
Comment 1 (Makefile): Created test-python-integration-dbt target that consolidates all the dbt test logic. Now you can run make test-python-integration-dbt locally!
Comment 2 (pytest.ini): The error::_pytest.warning_types.PytestUnhandledCoroutineWarning line was accidentally removed. It's been restored.
|
@YassinNouh21 can you post a screen shot of the feast UI and the dbt UI lineage graphs to see how they render? Also looks like you may have this hooked up to mcp to handle this, which is pretty cool |
|
@YassinNouh21 you can spin up the UI locally with |
|
hmm the protobuf issue sounds like a real concern, no? |


Summary
Test Coverage
Test plan