chore: refactor python tests. no functional changes just moving into separate files #1301
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 8f385b3 in 1 minute and 31 seconds
More details
- Looked at
3161lines of code in16files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. integ-tests/python/tests/caching_test.py:36
- Draft comment:
Consider using logging instead of print statements for better control over output and to integrate with larger applications. - Reason this comment was not posted:
Confidence changes required:20%
The refactoring seems to have been done correctly, with tests moved to separate files. However, there are some minor issues that can be improved for better code quality and maintainability.
2. integ-tests/python/tests/constraints_test.py:58
- Draft comment:
Remove print statements or replace them with logging for better control over output. - Reason this comment was not posted:
Confidence changes required:20%
The refactoring seems to have been done correctly, with tests moved to separate files. However, there are some minor issues that can be improved for better code quality and maintainability.
3. integ-tests/python/tests/dynamic_types_test.py:39
- Draft comment:
Consider removing print statements or replacing them with logging for better control over output. - Reason this comment was not posted:
Confidence changes required:20%
The refactoring seems to have been done correctly, with tests moved to separate files. However, there are some minor issues that can be improved for better code quality and maintainability.
4. integ-tests/python/tests/dynamic_types_test.py:47
- Draft comment:
Consider removing print statements or replacing them with logging for better control over output. - Reason this comment was not posted:
Confidence changes required:20%
The refactoring seems to have been done correctly, with tests moved to separate files. However, there are some minor issues that can be improved for better code quality and maintainability.
5. integ-tests/python/tests/dynamic_types_test.py:90
- Draft comment:
Consider removing print statements or replacing them with logging for better control over output. - Reason this comment was not posted:
Confidence changes required:20%
The refactoring seems to have been done correctly, with tests moved to separate files. However, there are some minor issues that can be improved for better code quality and maintainability.
6. integ-tests/python/tests/dynamic_types_test.py:122
- Draft comment:
Consider removing print statements or replacing them with logging for better control over output. - Reason this comment was not posted:
Confidence changes required:20%
The refactoring seems to have been done correctly, with tests moved to separate files. However, there are some minor issues that can be improved for better code quality and maintainability.
7. integ-tests/python/tests/dynamic_types_test.py:152
- Draft comment:
Consider removing print statements or replacing them with logging for better control over output. - Reason this comment was not posted:
Confidence changes required:20%
The refactoring seems to have been done correctly, with tests moved to separate files. However, there are some minor issues that can be improved for better code quality and maintainability.
8. integ-tests/python/tests/dynamic_types_test.py:277
- Draft comment:
Consider removing print statements or replacing them with logging for better control over output. - Reason this comment was not posted:
Confidence changes required:20%
The refactoring seems to have been done correctly, with tests moved to separate files. However, there are some minor issues that can be improved for better code quality and maintainability.
9. integ-tests/python/tests/constraints_test.py:11
-
Draft comment:
This is a duplicate implementation. Consider using the existingall_succeededfunction instead. -
function
all_succeeded(types.py.j2) -
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
This is a test file, and the implementation is slightly different from the one in types.py.j2. Test files often have helper functions that may duplicate production code for clarity or isolation. The difference in implementation (get_checks vs values()) suggests these might serve different purposes. Without more context about get_checks(), we can't be sure these are truly duplicates.
I might be too lenient on test code duplication. Perhaps having consistent helper functions across tests and production code would improve maintainability.
While consistency is valuable, test code often benefits from being self-contained and explicit. Without understanding get_checks(), suggesting a change could introduce unnecessary coupling.
The comment should be deleted as it's not clearly correct - we don't have enough context to know if these functions are truly duplicates, and test code often benefits from explicit, self-contained helpers.
Workflow ID: wflow_I3zpOpfKduesmsK0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 89e73ba in 29 seconds
More details
- Looked at
162lines of code in3files - Skipped
1files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/primary.yml:128
- Draft comment:
Consider using a versioned installer or a package manager to install Poetry to ensure security and integrity. - Reason this comment was not posted:
Confidence changes required:50%
The refactoring of the test files into separate files is well-executed, and the changes in the GitHub Actions workflow reflect the new structure. However, there is a minor issue in theprimary.ymlfile where thetest-pythonjob installs Poetry using a direct curl command. This is not a best practice as it can lead to security vulnerabilities. It's better to use a versioned installer or a package manager to ensure the integrity of the installation.
Workflow ID: wflow_CwlclK38KaPOexaz
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -8,4 +8,4 @@ env -u CONDA_PREFIX poetry run maturin develop --manifest-path ../../engine/lang | |||
| poetry run baml-cli generate --from ../baml_src | |||
|
|
|||
| # test_functions.py is excluded because it requires credentials | |||
There was a problem hiding this comment.
From this comment it looks like we didn't even run the integ tests?
| @@ -164,3 +164,5 @@ web_modules/ | |||
| yarn-debug.log* | |||
| yarn-error.log* | |||
| yarn.lock | |||
| .vars | |||
| test-report.html | |||
There was a problem hiding this comment.
I think we discussed about test-report.html and decided to not git ignore it.
|
Closing because it's out of date, and we can retry again later. |
Important
Refactor Python tests into separate files for better organization and update GitHub Actions workflow with environment variables and improved test setup.
test_functions.pyto individual files for better organization.caching_test.py,constraints_test.py,dynamic_types_test.py,error_handling_test.py,input_output_test.py,providers/,recursive_types_test.py,streaming_test.py,tracing_test.py.test_setup.pyfor common setup and teardown logic.providers/directory with tests for Anthropic, AWS, Gemini, and OpenAI.OPENAI_API_KEYandANTHROPIC_API_KEYtoprimary.yml.test_node_generatorjob totest-typescriptand update steps for TypeScript client build and test.This description was created by
for 89e73ba. It will automatically update as commits are pushed.