Let tests fail if there is a javascript error NOW #5225
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements JavaScript error detection in screen tests to prevent JavaScript errors from silently failing during testing. The main purpose is to ensure that tests fail when JavaScript errors occur, improving test reliability and catching frontend issues early.
Key changes:
- Added JavaScript error checking in the test teardown process
- Added an
allow_js_errorsflag to selectively disable error checking for tests that expect JS errors - Updated specific tests to allow JS errors where they are expected (error pages, sanitization tests, etc.)
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| nicegui/testing/screen_plugin.py | Added JavaScript error detection logic in test teardown |
| nicegui/testing/screen.py | Added allow_js_errors flag to Screen class |
| tests/test_storage.py | Allowed JS errors for storage test that accesses missing route |
| tests/test_page.py | Allowed JS errors for exception handling and timeout tests |
| tests/test_html.py | Allowed JS errors for HTML sanitization test |
| tests/test_endpoint_docs.py | Added proper page definitions to prevent JS errors |
| tests/test_element.py | Removed emoji from valid tags list |
| tests/test_chat_message.py | Allowed JS errors for chat message test |
| nicegui/elements/echart/echart.js | Fixed potential undefined access with optional chaining |
| js_errors = [e for e in browser_logs if str(e.get('level', '')).upper() in ( | ||
| 'SEVERE', 'ERROR')] | ||
| if js_errors and not screen_.allow_js_errors: | ||
| pytest.fail(f'JavaScript console errors:\n{js_errors}', pytrace=False) |
There was a problem hiding this comment.
The error message format {js_errors} will display the raw list representation which is hard to read. Consider formatting the errors more clearly, such as joining individual error messages or extracting relevant fields like message and source.
| pytest.fail(f'JavaScript console errors:\n{js_errors}', pytrace=False) | |
| formatted_errors = "\n".join( | |
| f"- {e.get('message', str(e))}" for e in js_errors | |
| ) | |
| pytest.fail(f'JavaScript console errors:\n{formatted_errors}', pytrace=False) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Do not do that. Want full context.
|
|
||
| screen.open('/') | ||
| assert get_openapi_paths() == set() | ||
| assert get_openapi_paths() == {'/'} |
There was a problem hiding this comment.
This assertion change from set() to {'/'} suggests the test expectation has changed. However, the test name test_endpoint_documentation_page_only implies it should only document pages, but now it expects the root path to be documented. Verify this is the intended behavior.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Intended behaviour. Since added @ui.page('/') to all tests, thetest_endpoint_documentation_page_only would have {'/'} in get_openapi_paths()
falkoschindler
added
testing
2 participants
Motivation
As I discovered in #5143, current screen tests let javascript errors slide.
Real motivation: #5146 stale and merge-conflict. I stole the PR desription from there, if it isn't obvious
Implementation
Simple addition to the teardown of screen which checks for javascript errors.
But in line 91 I did
{js_errors}instead. Maybe that's why in #5146 we have WAY MORE logs than what we should expect.Progress