Conversation
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (50.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #108 +/- ##
===========================================
+ Coverage 53.20% 76.34% +23.13%
===========================================
Files 19 29 +10
Lines 374 816 +442
Branches 19 21 +2
===========================================
+ Hits 199 623 +424
- Misses 171 191 +20
+ Partials 4 2 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a3053a7 to
37b39e5
Compare
37b39e5 to
f020a50
Compare
|
@cheryldmorse I didn't feel like I got this to a ready to review/land state before switching over to #109 so let me know if you have any questions, want to take it over, or want me to do anything specific before you run with #101 |
|
Ok, now at least the tests are running enough to fail instead of failing because they weren't found. xref #119 |
20316dc to
e48de07
Compare
Works on #101
Adds makefile command to help out with testing.
e48de07 to
bce3af6
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds initial testing infrastructure for S3 timeseries pipelines, including test scaffolding, sensor implementation for detecting new S3 files, and Makefile commands to facilitate testing across the project.
Changes:
- Adds comprehensive test infrastructure with pytest configuration, fixtures, and test data for Empire met station
- Implements S3 sensor to automatically detect and process new files uploaded to S3 buckets
- Migrates from pixi.toml to pyproject.toml for better Python ecosystem integration
- Adds Makefile targets (
test-all,test-s3-timeseries, etc.) for easier test execution
Reviewed changes
Copilot reviewed 13 out of 16 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| pipeline/s3_timeseries/tests/test_empire_met.py | New test file with fixtures and tests for sensor, daily, and monthly assets |
| pipeline/s3_timeseries/tests/test_data/empire_met/*.csv,.nc | Snapshot test data files for Empire met station |
| pipeline/s3_timeseries/tests/conftest.py | Pytest configuration adding --aws flag for integration tests |
| pipeline/s3_timeseries/pyproject.toml | New project config replacing pixi.toml with dependencies and pytest settings |
| pipeline/s3_timeseries/pixi.toml | Removed in favor of pyproject.toml |
| pipeline/s3_timeseries/pipeline.py | Adds S3 sensor implementation and data processing improvements |
| pipeline/s3_timeseries/Dockerfile | Updated to copy pyproject.toml and test files |
| common/test_utils/sensors.py | New utility for extracting sensors from Definitions |
| common/test_utils/init.py | Exports get_sensor_by_name utility |
| Makefile | Adds test-common, test-hohonu, test-s3-timeseries, and test-all targets |
| README.md | Updated testing documentation with new commands |
| .gitignore | Adds junit.xml to ignored files |
| .github/workflows/pipeline_s3_timeseries.yml | Enables test execution with coverage reporting in CI |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client = boto3.client( | ||
| "s3", | ||
| aws_access_key_id=s3_credentials.access_key_id, | ||
| aws_secret_access_key=s3_credentials.secret_access_key, |
There was a problem hiding this comment.
The boto3 client is created without specifying a region_name parameter. This could lead to unexpected behavior or errors depending on the environment configuration. The S3FSResource fixture uses "us-east-1", but the sensor's boto3 client in line 241-245 does not specify a region, which is inconsistent. Consider adding a region_name parameter to the boto3.client() call for consistency and to ensure the sensor works correctly in all environments.
| aws_secret_access_key=s3_credentials.secret_access_key, | |
| aws_secret_access_key=s3_credentials.secret_access_key, | |
| region_name="us-east-1", |
|
@abkfenris I've opened a new pull request, #122, to work on those changes. Once the pull request is ready, I'll request review from you. |
Adds some initial scaffolding for testing S3 timeseries pipelines.
This pipeline is likely to end up pretty complex in order to handle all the different ways that our data providers will provide us data, so we need to be able to make sure we don’t have major regressions in existing data processing when we add new capabilities.
This additionally adds Makefile commands to help out with testing.
make test-allto run all of our tests, ormake test-<thing>to try to test something specific.Works on #101
This is part 1 of 3 in a stack made with GitButler: