Repository navigation
refactor(tests): remove test smells from sync tests - #3309
Closed
Cauan Ricardo (cauanrricardo) wants to merge 42 commits into
Closed
Cauan Ricardo (cauanrricardo) wants to merge 42 commits into
Cauan Ricardo (cauanrricardo) wants to merge 42 commits into
Conversation
…est_locators_should_query_existing_elements
…est_locator_all_should_work
…ndler_times_out (Exception Handling + Non-Functional Statement)
…o_set_custom_global_timeout
…uld_not_chain_abort
…default_should_abort_requests_not_found_in_har
…n_max_redirects_is_exceeded
…ror_when_max_redirects_is_exceeded
…ts_when_max_redirects_is_set_to_0
Replace the try/finally around screencast.start() with a with block, so stop() runs on exit without manual exception handling. Smell: Exception Handling
Compare browser_type.name directly with the browser_name fixture instead of branching on is_webkit/is_firefox/is_chromium, so the test has a single assertion path for every browser. Smell: Conditional Test Logic
The inline loop duplicated the ensure_some_frames helper already defined in this file (100 paint cycles followed by a screenshot), so call it instead of repeating the loop. Smell: Conditional Test Logic
Assert the viewport height and timestamp type of every received frame with all() over the frames instead of looping with assertions inside. Smell: Conditional Test Logic
The pointer-cursor show_actions block had an empty pass body. Render a button and click it inside the block so the test exercises show_actions instead of entering and leaving it without doing anything. Smell: Non-Functional Statement
The cursor="none" show_actions block also had an empty pass body. Click the same button inside it so both cursor values are exercised by a real action. Smell: Non-Functional Statement
Replace the try/except plus assert False with pytest.raises(Error), so the test fails loudly if evaluate() does not raise, and check the error message afterwards. Smell: Exception Handling
Replace the try/finally around the first screencast.start() with a with block, so stop() runs on exit while pytest.raises keeps checking the "already started" error. Smell: Exception Handling
…n_max_redirects_is_less_than_0
…oes_not_encode_unicode
…n test_should_delete_header_with_undefined_value
…ponse_with_text_content_type_if_to_be_ok_fails
… test_should_attach_content
Replace the try/finally around screencast.start() with a with block, so stop() runs on exit without manual exception handling. Smell: Exception Handling
Give ensure_some_frames a count parameter (default 100, so other callers are unchanged) and use it with count=3 instead of repeating the inline paint-cycle loop followed by a screenshot. Smell: Conditional Test Logic
Run test_scroll once per offscreen button with pytest.mark.parametrize instead of looping inside a single test. Each case now gets a fresh page, so the scrollTo(0, 0) reset between iterations is no longer needed. Smell: Conditional Test Logic
Check each closed page with its own pytest.raises block instead of looping over a list of pages, so a failure points at the exact page. Smell: Conditional Test Logic
test_should_set_local_storage verified two unrelated things in about 50 lines. Move the shared storage state, context and page setup into a fixture and check localStorage and IndexedDB in separate tests. Smell: Verbose Test
Replace the index loop with one assertion comparing all last-modified timestamps with pytest.approx(abs=1000). The tolerance is the same, but the bound is now inclusive (<= 1000 instead of < 1000). Smell: Conditional Test Logic
Choose the expected console message text with a dict lookup keyed by browser_name instead of an if/else, keeping the same test and the same assertions for every browser. Smell: Conditional Test Logic
Choose the expected console message text with a dict lookup keyed by browser_name instead of an if/else, keeping the same test and the same assertions for every browser. Smell: Conditional Test Logic
Replace the if/else on browser_name with a lookup of the expected text, keeping the same test, ID and assertion. Smell: Conditional Test Logic
The try/finally that stopped the HTTPServer now lives in a yield fixture, so the timing test only holds the assertions. Assertions are unchanged. Smell: Exception Handling
…r tests Move the engine registration and page setup into a fixture and split the long test into main world, isolated world and chaining tests. Every assertion is kept as is. Smell: Verbose Test
Move the deadline while loop to wait_for_received_frames, next to ensure_some_frames. Same 10s deadline and 100ms polling step, so timing is unchanged. Smell: Conditional Test Logic
…uld_not_chain_abort (page_request_fallback)
Author
|
@microsoft-github-policy-service agree |
Author
|
Note on merge conflicts: this work was done against commit 3705559 (the base of this branch). Since then, main reorganized and rewrote the test suite (#3276, The changes are small, independent commits (one refactoring each), so they can be ported to the new layout. If the maintainers are interested, we are happy to rebase or re-apply them on top of the current main, or split them into smaller PRs. Linked issue: #3308. |
|
@microsoft-github-policy-service agree |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3308
What
Refactors 41 test smell occurrences across 16 files in
tests/sync/, one commit per refactoring, plus one formatting commit (black). Smells addressed (detected with TEMPY): Conditional Test Logic (25), Exception Handling (7), Verbose Test (6) and Non-Functional Statement (3).Why
Conditional logic,
try/exceptinside tests and very long tests make tests harder to read and to diagnose when they fail. The refactorings make each test a linear flow (dict lookups, parametrization,pytest.raises, fixtures, small helpers) while verifying the same behavior.How tested
skip_browseron split browser-specific tests.black,isortandflake8pass on all changed files (versions from.pre-commit-config.yaml).Notes for reviewers
Authors
test_screencast.py,test_sync.py,test_launcher.py,test_element_handle.py,test_console.py,test_fetch_browser_context.py,test_browsercontext_storage_state.py,test_queryselector.pyandtest_browsertype_connect.pytest_assertions.py,test_browsercontext_request_fallback.py,test_fetch_global.py,test_har.py,test_locators.py,test_page_request_fallback.pyandtest_page_add_locator_handler.py