[chore] Add unit tests to improve Python coverage#14479
Conversation
- Add errors_test.py covering error exception classes - Add image_utils_test.py covering image utility functions - Extend write_test.py with StringIO handling tests - Extend string_util_test.py with edge case tests - Extend url_util_test.py with raw GitHub URL tests Co-Authored-By: Claude Opus 4.6 <[email protected]>
lukasmasuch
added
impact:internal
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
lukasmasuch
added
the
ai-review
✅ PR preview is ready!
|
There was a problem hiding this comment.
Pull request overview
Adds/extends Python unit tests to raise coverage across several Streamlit utility modules, supporting the repo’s 90%+ coverage goal without changing production behavior.
Changes:
- Add new unit test suites for
streamlit.errorsandstreamlit.elements.lib.image_utils. - Extend existing unit tests for
st.write,string_util, andurl_utilwith additional edge cases.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/tests/streamlit/write_test.py |
Adds coverage for st.write handling io.StringIO. |
lib/tests/streamlit/url_util_test.py |
Adds a regression test ensuring GitHub .../raw/... URLs remain unchanged. |
lib/tests/streamlit/string_util_test.py |
Adds edge-case tests for emoji/material icon validation, binary detection, and from_number error paths. |
lib/tests/streamlit/errors_test.py |
New tests covering exception/warning classes and key message formatting behaviors in streamlit.errors. |
lib/tests/streamlit/elements/lib/image_utils_test.py |
New tests covering internal image utilities: format selection, numpy shape validation, and clipping behavior. |
github-actions
Bot
removed
the
ai-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR adds new unit tests and extends existing ones to improve Python code coverage for errors.py, image_utils.py, string_util.py, url_util.py, and st.write (StringIO handling). Additionally, the PR removes the Windows-specific file-watcher stability check from event_based_path_watcher.py along with its corresponding tests. While the test additions are well-structured, both individual reviewers characterized this as a "test-only" PR, overlooking the production code removal in the watcher module.
Reviewer consensus: 2 of 3 expected models completed reviews (gemini-3.1-pro and gpt-5.3-codex-high). claude-4.6-opus-high-thinking failed to complete its review. gemini-3.1-pro requested changes (local import nit); gpt-5.3-codex-high approved. Both reviewers agreed on code quality, no security risk, no external-test need, and no accessibility impact — but both missed the production code change.
Code Quality
The test additions are well-structured, following project conventions with clear docstrings, appropriate assertions, and good edge-case coverage. Minor style issue: from io import StringIO is imported locally inside two test functions in write_test.py (lines 779, 791) rather than at the top of the file, which contradicts lib/tests/AGENTS.md guidelines. This is a low-severity nit covered by the inline comment.
Production code change (missed by both reviewers): The diff removes ~68 lines of production code from lib/streamlit/watcher/event_based_path_watcher.py, specifically the Windows-specific stability check that mitigated spurious file-change events from Windows Defender, Search Indexer, and OneDrive (linked to issue #13954). The corresponding _WINDOWS_STABILITY_DELAY_SECS constant and the module-level docstring describing the Windows considerations are also removed, along with ~145 lines of related tests. This is a functional behavior change on Windows, not a test-only change.
Test Coverage
The new tests provide meaningful coverage improvements for the targeted modules:
errors_test.py(new): Covers exception classes and message formatting.image_utils_test.py(new): Covers format normalization, ndarray shape validation, and clipping range behavior.string_util_test.py(extended): Adds edge-case checks forNoneicon input, binary detection, andfrom_numberfailure paths.url_util_test.py(extended): Adds regression test for already-raw GitHub URLs.write_test.py(extended): Adds coverage forStringIOhandling inst.write.
The removal of 3 Windows-specific watcher tests is appropriate given the corresponding production code was removed. However, the net effect reduces test coverage for Windows file-watching behavior.
Backwards Compatibility
The test additions carry no backwards compatibility risk. The removal of the Windows stability check in event_based_path_watcher.py could cause regressions on Windows where background processes (Windows Defender, Search Indexer, OneDrive) trigger spurious file-change events, potentially leading to unnecessary app reruns. This should be confirmed as intentional.
Security & Risk
No security concerns identified. The changes do not touch auth, session management, networking, or other security-sensitive areas. Regression risk from test additions is negligible. The watcher code removal carries minor regression risk limited to Windows platforms.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence:
- Test files only exercise internal utility functions and error classes.
- The watcher production code removal is internal file-watching behavior, not related to routing, auth, embedding, or cross-origin behavior.
- Suggested external_test focus areas: N/A
- Confidence: High
- Assumptions and gaps: Assessment assumes the Windows stability check removal was intentionally decided outside this PR's scope.
Accessibility
N/A — No frontend or UI changes are included in this PR.
Recommendations
- Move
from io import StringIOto the top-level imports inlib/tests/streamlit/write_test.pyperlib/tests/AGENTS.mdguidelines (both occurrences at lines 779 and 791). - Confirm the removal of the Windows-specific stability check in
event_based_path_watcher.py(and its tests) is intentional. If this was a revert of a prior change, a brief note in the PR description would help reviewers understand the context and link back to issue #13954.
Verdict
CHANGES REQUESTED: The test additions are solid, but the StringIO import should be moved to the top of the file per project guidelines, and the production code removal (Windows watcher stability check) should be explicitly acknowledged in the PR description to confirm it is intentional — both reviewers missed this functional change.
This is a consolidated AI review by claude-4.6-opus-high-thinking, synthesizing reviews from gemini-3.1-pro and gpt-5.3-codex-high. claude-4.6-opus-high-thinking failed to complete its individual review. Please verify the feedback and use your judgment.
This review also includes 1 inline comment(s) on specific code lines.
📈 Python coverage change detectedThe Python unit test coverage has increased by 0.1176%
🎉 Great job on improving test coverage! Coverage by files
|
…parameterized test - Move `from io import StringIO` to top-level imports as per test guidelines - Combine two identical StringIO tests into a single parameterized test Co-Authored-By: Claude Opus 4.6 <[email protected]>
lukasmasuch
added
the
ai-review
github-actions
Bot
removed
the
ai-review
There was a problem hiding this comment.
Summary
This PR adds and extends Python unit tests to improve code coverage (~0.17% increase) toward the 90%+ coverage goal. No production code is modified — only test files are added or extended. The changes cover:
- New file
errors_test.py: 14 tests for error/exception classes instreamlit/errors.py - New file
image_utils_test.py: 19 tests for image utility functions (_validate_image_format_string,_verify_np_shape,_clip_image) - Extended
write_test.py: StringIO handling inst.write - Extended
string_util_test.py: Edge cases forvalidate_emoji,validate_material_icon,is_binary_string, andfrom_number - Extended
url_util_test.py: GitHub raw URL handling inprocess_gitblob_url
Reviewer consensus: All three reviewers (claude-4.6-opus-high-thinking, gemini-3.1-pro, gpt-5.3-codex-high) approved this PR unanimously. No critical or blocking issues were raised.
Code Quality
The tests are well-structured, readable, and follow existing patterns in the codebase. Each test has a clear docstring and targeted assertions. All reviewers agreed on the high quality of the test code.
Minor style observations (all non-blocking):
- The two new test files use pytest-compatible classes for grouping, whereas
lib/tests/AGENTS.mdprefers standalone pytest functions with@pytest.mark.parametrize. Two reviewers flagged this; one did not. The class-based approach works correctly with pytest and is internally consistent, so this is a style preference only. - New test methods added to legacy
unittest.TestCasefiles lack return type annotations (-> None). One reviewer noted this deviates from current guidance, while another noted it is consistent with the surrounding existing methods. Either approach is reasonable for legacy files.
Test Coverage
All reviewers agreed the tests are well-targeted and cover meaningful code paths:
errors_test.py:__repr__, stack capture, message formatting, conditional error messages (uses_pages_directorybranches), and mixed numeric type error parameters.image_utils_test.py: Format validation (explicit formats + auto-detection), numpy shape validation (valid shapes, invalid dimensions, channel conversion), and image clipping (float/int ranges, clamping, error cases).string_util_test.py: None-input edge cases for validators, binary string detection, andfrom_numberwith non-numericitem()return values.url_util_test.py: Raw GitHub URLs pass through unchanged.write_test.py: StringIO → markdown rendering path.
One area where assertion strength could be improved: the _clip_image tests only check min()/max() bounds rather than exact array values. Using np.testing.assert_array_equal would provide stronger regression protection (see inline comment).
Backwards Compatibility
No concerns. This PR only adds tests — no source code, APIs, protobufs, or frontend code are modified. All reviewers unanimously agreed there is zero risk of breaking existing functionality.
Security & Risk
No security concerns. All reviewers agreed the changes are entirely within the test suite and do not affect runtime code, dependencies, or configuration.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence:
- All changed files are in
lib/tests/streamlit/— pure unit test additions with no runtime impact - No changes to routing, auth, WebSocket, embedding, assets, CORS, security headers, or any other external-facing behavior
- All changed files are in
- Suggested external_test focus areas: None
- Confidence: High (all three reviewers agreed)
- Assumptions and gaps: None
Accessibility
Not applicable — no frontend changes. All reviewers agreed.
Recommendations
-
Strengthen
_clip_imageassertions: Usenp.testing.assert_array_equalto verify exact expected array values instead of justmin()/max()bounds intest_float_image_with_clamp,test_float_image_without_clamp_valid_range, andtest_int_image_with_clamp. (Raised by gemini-3.1-pro, confirmed on review.) -
Document
min_value=0falsy behavior: Inerrors_test.pyline 127,min_value=0is falsy, so the source code'sif min_value:check silently omits it from the error message. Adding an explicit negative assertion (e.g.,assert "min_value" not in str(error)) would codify this behavior. (Raised by claude-4.6-opus-high-thinking.) -
Consider standalone pytest functions for new files: For
errors_test.pyandimage_utils_test.py,lib/tests/AGENTS.mdprefers standalone pytest functions with@pytest.mark.parametrizeover class-based grouping. Low priority and non-blocking.
Verdict
APPROVED: Clean, well-structured test-only PR that meaningfully improves Python code coverage across five modules with no risk to existing functionality. All three reviewers approved unanimously with no blocking issues.
Consolidated AI review by claude-4.6-opus-high-thinking. Individual reviews: claude-4.6-opus-high-thinking (APPROVED), gemini-3.1-pro (APPROVED), gpt-5.3-codex-high (APPROVED). All expected models completed their reviews successfully.
This review also includes 3 inline comment(s) on specific code lines.
…parameterized test - Remove errors_test.py (errors.py excluded from coverage) - Refactor image_utils_test.py to use standalone pytest functions with @pytest.mark.parametrize Co-Authored-By: Claude Opus 4.6 <[email protected]>
…ude trivial ones - Remove errors.py from coverage omit (was hiding real coverage gaps) - Add pragma: no cover to trivial exception classes (no logic to test) - Add focused tests for classes with non-trivial logic: - LocalizableStreamlitException (exec_kwargs property) - StreamlitAPIWarning (captures stack trace) - StreamlitMixedNumericTypesError (conditional message building) - StreamlitPageNotFoundError (conditional message) - StreamlitSelectionCountExceedsMaxError (pluralization) - StreamlitInvalidMaxError (optional corrective action) Co-Authored-By: Claude Opus 4.6 <[email protected]>
| assert str(exc) == "Value 42 is invalid for test_param" | ||
|
|
||
|
|
||
| def test_localizable_exception_exec_kwargs_property(): |
There was a problem hiding this comment.
Nit: Added tests in this file don't follow the pattern in image_utils_test.py above which have the -> None return on test functions
There was a problem hiding this comment.
Added these here as well 👍
| ), | ||
| ], | ||
| ) | ||
| def test_clip_image_valid(array: np.ndarray, clamp: bool, check_fn) -> None: |
There was a problem hiding this comment.
suggestion: The lambda-based check_fn assertions could be improved. For deterministic operations like clamping + scaling, checking only min() >= 0 and max() <= 255 would still pass if clipping accidentally zeroed all values. Consider using np.testing.assert_array_almost_equal with the exact expected output (e.g., [[0, 127.5], [255, 204]] for the float-clamp case). The int-without-clamp case already does this well with np.array_equal.
There was a problem hiding this comment.
Changed to use np.testing.assert_array_almost_equal 👍
| """Test message when all numeric args have different types.""" | ||
| exc = errors.StreamlitMixedNumericTypesError( | ||
| value=1.0, | ||
| min_value=1, # Use non-zero to ensure it's included |
There was a problem hiding this comment.
question: outside the scope of this PR, but perhaps worth a follow up - the comment here acknowledges that min_value=0 would be silently excluded because the source uses if min_value: (falsy check) rather than if min_value is not None: - should these just be fixed?
There was a problem hiding this comment.
Changed it to if min_value is not None:
…zero-value bug - Add `-> None` return type annotations to all test functions in errors_test.py - Replace weak min/max bound checks with exact value assertions in image_utils_test.py - Fix StreamlitMixedNumericTypesError to use `is not None` checks instead of truthiness, so zero values are correctly included in error messages - Add test for zero-value handling in mixed numeric types error Co-Authored-By: Claude Opus 4.6 <[email protected]>
Describe your changes
Adds unit tests to improve Python code coverage by ~0.17%, targeting 90%+ coverage goal:
errors_test.pywith 14 tests covering error exception classesimage_utils_test.pywith 19 tests covering image utility functionswrite_test.pywith StringIO handling testsstring_util_test.pywith edge case tests for validation functionsurl_util_test.pywith raw GitHub URL handling testsFiles improved:
lib/streamlit/errors.py: 87% → 100%lib/streamlit/elements/lib/image_utils.py: 88.1% → 93.2%lib/streamlit/string_util.py: 91.5% → 99.1%lib/streamlit/url_util.py: 90.5% → 92.9%GitHub Issue Link (if applicable)
N/A
Testing Plan
lib/tests/streamlit/errors_test.py— Tests error exception classeslib/tests/streamlit/elements/lib/image_utils_test.py— Tests image utility functionslib/tests/streamlit/write_test.py— Tests StringIO handling in st.writelib/tests/streamlit/string_util_test.py— Tests validation functionslib/tests/streamlit/url_util_test.py— Tests GitHub URL processingAgent metrics