-
Notifications
You must be signed in to change notification settings - Fork 228
Add comprehensive test coverage and GitHub Actions integration testing #209
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: main
Are you sure you want to change the base?
Add comprehensive test coverage and GitHub Actions integration testing #209
Conversation
- Fix existing unit test failures across all test files - Add comprehensive Azure DevOps pipeline test coverage - Fix markdown link checker tests by adding ignore patterns for broken links - Add integration test suite for end-to-end MLOps stack testing - Add GitHub Actions workflow for automated integration testing with AWS/Azure support - Update main test workflow with Node.js v22 for markdown link compatibility - Configure workflows to use Databricks-hosted runners for secure workspace access - Fix Black formatting issues across all test files This enables automated nightly integration testing and improves overall test coverage
Remove config file from test log artifacts to streamline uploaded content
Simplify cleanup logic
|
|
||
| jobs: | ||
| integration-tests: | ||
| runs-on: |
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.
FYI: This is temporary, we will need to work on dedicated infra for this since we have integration tests now
- Use existing current_user fixture instead of slow CLI calls - Remove unused deployed_project_path dependency
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.
Bit difficult to review a 4000 line PR, can we break these up into multiple PRs (maybe one per test suite) and I'll review one at a time? I'll go in order and that way comments on one PR can be propagated to future PRs, preventing huge rewrites on your end from one systemic thing
| on: | ||
| schedule: | ||
| # Run nightly at 2 AM UTC | ||
| - cron: '0 2 * * *' |
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.
Hmm instead of nightly, we can just run (with approval from a maintainer) on PRs
|
|
||
| notify-failure: | ||
| runs-on: | ||
| group: databricks-field-eng-protected-runner-group |
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.
Not sure if we can access these groups from public repos (haven't been able to in the past)
| username: ${{ secrets.NOTIFICATION_EMAIL_USERNAME }} | ||
| password: ${{ secrets.NOTIFICATION_EMAIL_PASSWORD }} | ||
| subject: "FAILED: MLOps Stacks Integration Tests Failed - ${{ github.sha }}" | ||
| to: ${{ secrets.NOTIFICATION_EMAIL_TO }} |
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.
Which email is this to?
| needs: integration-tests | ||
| if: failure() && github.event_name == 'schedule' | ||
| steps: | ||
| - name: Send Email Notification |
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.
I don't think we need email notification failures if we end up just running these on PRs, just leave a comment on the PR?
| @@ -0,0 +1,2 @@ | |||
| # Integration tests for MLOps Stacks | |||
| # These tests require a real Databricks workspace and are configured via CLI profiles | |||
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.
Probably a comment why this file is empty helps
|
|
||
|
|
||
| def _cleanup_unity_catalog_model(databricks_cli, workspace_config, project_name): | ||
| """Clean up Unity Catalog models by finding and deleting all matching models.""" |
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 don't we just use the Databricks SDK instead of running all these subprocesses?
This enables automated nightly integration testing and improves overall test coverage