-
Notifications
You must be signed in to change notification settings - Fork 4
Add UpliftAI key to CI tests #65
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| # Instructions for Contributors | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 | Confidence: Medium The documentation correctly captures the development workflow but lacks specific guidance on handling missing credentials. The test command uses |
||
|
|
||
| - Use [`uv`](https://github.com/astral-sh/uv) for dependency management. | ||
| Install all extras with: | ||
|
|
||
| ```bash | ||
| uv sync --all-extras | ||
| ``` | ||
|
|
||
| - Run tests with: | ||
|
|
||
| ```bash | ||
| uv run pytest -m "not synthetic and not sapi and not watson" | ||
| ``` | ||
|
|
||
| - The test suite requires credentials for several TTS engines. Provide | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2 | Confidence: High This instruction is clear but incomplete. It doesn't specify whether contributors should also update the test command's filter flags (e.g., Code Suggestion: Expand the documentation to include guidance on updating test filters when adding new engines, and consider linking to the pytest marker definitions for clarity. |
||
| the necessary environment variables or a `credentials.json` file as | ||
| described in `tests/load_credentials.py`. | ||
|
|
||
| - When new engines require credentials, ensure corresponding secrets are | ||
| added to the GitHub Actions workflow environment variables. | ||
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.
[Contextual Comment]
This comment refers to code near real line 83. Anchored to nearest_changed(95) line 95.
P2 | Confidence: High
The addition of
UPLIFTAI_KEYfollows the existing pattern for secret management, which is good. However, the workflow does not include any validation to ensure required secrets are present before attempting tests. If a secret is missing (e.g., not configured in GitHub), the tests will fail with cryptic errors rather than clear feedback about missing environment variables. This could slow down troubleshooting for maintainers.Code Suggestion:
Consider adding a pre-test step to validate that all required secrets are non-empty, or implement graceful skipping of tests for missing credentials with clear log messages.