test: convert the last unittest classes onto pytest fixtures (PyTest - 11) - #1694
Open
Manik-Khajuria-5 wants to merge 57 commits into
Open
test: convert the last unittest classes onto pytest fixtures (PyTest - 11)#1694Manik-Khajuria-5 wants to merge 57 commits into
Manik-Khajuria-5 wants to merge 57 commits into
Conversation
Adds conftest.py, a testutils package, and unit tests for window().
AsyncHTTPTestCase is a unittest.TestCase, and pytest will not inject fixtures into those. Run the Application on a background event loop instead and talk to it with requests, which is already a runtime dependency, so HTTP tests can be plain functions.
Lands the window lifecycle file as plain pytest functions on the visdom_server fixture, dropping the per-file setUp block and the temp directory it never cleaned up.
Collapses the repeated plot-trace and opts assertions into parametrized tables, so each case reports separately instead of hiding behind the first failure.
The reload case now builds its second Application from the app_factory fixture, which shares env_path with visdom_server, instead of hand-rolling one against a leaked temp directory.
The update-into-a-missing-env case asserted 'status is 200 or 500', which could not fail. It now pins the actual behaviour: 200 with 'win does not exist'.
register_window took the index from len(env), so closing any window but the last produced a duplicate index and the pane order went undefined after a reload. Use max(i) + 1, which is what the undo path already does.
Three validation checks used a bare assert, so an invalid request came back as a 500 and, under python -O, was skipped entirely: forking an unknown env then raised KeyError and a named trace update read past the data it was given. Raise HTTPError(400) like the other checks do.
Places the two test files dev added into the new layout, and points the new storage-wiring test at env_payload: it called the local _env helper this branch had already replaced, which merged cleanly and then failed.
Brings in dev along with the layout placement and the env_payload fix made on the PR-1 branch. D1 and D2 both survive the auto-merge of server_utils.py and web_handlers.py.
Replaces the background event loop and requests session with the AsyncHTTPTestCase base class from PR-1, which already runs the app in-process on an ephemeral port. The four integration files become TestCase subclasses; pytest still collects and marks them.
first/second read as two windows; they are the id returned by each call, which is the same id both times. Also assert only one pane exists.
The guidance still said VisdomHTTPTestCase was scheduled for replacement by a background-loop fixture. Record the opposite, and the consequences for anyone writing an HTTP test: no fixtures, no parametrize, share via a base class.
Both halves now run off VisdomHTTPTestCase instead of their own fixture block.
The three cap tables collapse onto parametrize; the file was untracked until now.
Guard the missing-point check behind a numeric test, and stop indexing data and traces past their length when an update supplies fewer entries than the plot.
It was the one history pane that grew without bound.
Removing a heatmap names no trace and posts no data, so the shortcut for opts-only updates returned before the delete branch and left the plot up.
write_error rendered the exception, its traceback and request.__dict__ whenever tornado's debug flag was set, and app.py set that flag unconditionally -- so every deployment served its own source paths and request internals on any 500, and error.html's production branch was unreachable. The flag also turned on autoreload for every server as a side effect. Replaces it with a show_error_details setting driven by the root logging level, so -logging_level DEBUG brings the detail back and nothing else changes with it.
TestClientMessageShapes built its client with Visdom(send=False) but left use_incoming_socket at its default, so every one of its six tests opened an incoming socket and waited out the connect timeout: 6.30s each, 37.8s of the file's 37.99s and of the whole suite's 43s. Those six drive no Application and no HTTP, so they belong in unit/ on the offline_client fixture rather than here; they move to unit/client_content.py. The two AsyncHTTPTestCase classes stay TestCases, as the HTTP round trip requires, and lose their duplicated setUp/get_app/post_json to VisdomHTTPTestCase. The readonly class expresses its server through the app_kwargs hook the base class already provides. The file now runs in 0.17s instead of 37.99s.
Adds unit/client_content.py: text, properties, table, embeddings and its event handlers, learning_curve, update_window_opts, the window and env writes, and the four methods that parse the server's reply rather than returning it. The experiment message-shape tests move here from integration/. The reply parsers cannot use capture_send -- with send=False, _send returns a (msg, endpoint) tuple and json.loads chokes on it -- so they patch _send with a canned reply instead. Fixes get_env_list, which was documented and typed as returning a list of env names but returned the current env's pane dict. _send defaults a missing eid to the client's env, and /env_state answers with one env's windows whenever it is given an eid, returning the env list only when it is not. A new default_eid=False keyword lets a caller opt out of that defaulting; every route that wants the current env is unaffected. Two behaviours are pinned rather than changed: properties validates none of the five property types it documents, and embeddings raises on a client with no incoming socket unless opts.register_embedding_events is False.
Eight files carried no module-level marker, so -m unit and -m integration selected 1165 of 1428 tests between them and 263 were in neither bucket. With the markers backfilled the two selections now sum to the whole suite, which is the property the new CI split depends on. unit/server_utils.py was also missing the license header AGENTS.md requires. python-tests.yml gains a unit job that runs in about a second and gates the 3.12/3.13 matrix, so an obvious break fails before three torch installs happen. The matrix job now runs with --cov-fail-under=80 against a measured 84%; the gap is deliberate headroom so an unrelated PR does not go red on rounding. The flags live in the workflow rather than in pyproject's addopts, because a floor is meaningless on the single-file runs done while writing a test and addopts would make every local pytest hard-require pytest-cov. Documents the marker meanings, the bucket-sum invariant and the socket timeout that made the suite slow, so the next person does not reintroduce it.
Drops --cov-fail-under from the workflow. CI still runs --cov=visdom --cov-report=term-missing, so the number is on every run and the next PR can pick a threshold from real data instead of guessing one. Choosing that threshold is its own decision and does not belong bundled with the suite work. The candidates are not equivalent: 84 is an exact ratchet with no slack for the fact that CI has torch and av installed where a dev machine may not, and 100 is unreachable without either covering the sklearn logger -- whose autolog() monkey-patches every estimator with no un-patch API -- or pragma-ing out roughly 500 lines, which would make the number meaningless. The docs now say coverage is reported rather than enforced, and record what a future floor has to account for.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @Manik-Khajuria-5, your pull request is larger than the review limit of 150000 diff characters
Manik-Khajuria-5
requested review from
Jayantparashar10,
Saksham-Sirohi,
marcoag,
mariobehling,
norbusan,
tonypzy and
vedansh-5
August 7, 2026 09:54
Contributor
Reviewer's GuideRefactors the test suite and server utilities to fully migrate Python unit tests to pytest with shared fixtures, add extensive unit and integration coverage for client and server behavior (including plots, media, embeddings, sockets, env lifecycle, storage, and t-SNE), tighten server correctness and robustness (read-only enforcement, auth, error handling, caps on histories, atomic env writes), adjust pytest and CI configuration, and document the new testing strategy. Sequence diagram for readonly HTTP write rejectionsequenceDiagram
actor Client
participant PostHandler
participant check_auth
participant check_readonly
participant reject_readonly
Client->>PostHandler: post()
activate PostHandler
PostHandler->>check_auth: _check_auth(handler)
activate check_auth
check_auth-->>PostHandler: auth ok
deactivate check_auth
PostHandler->>check_readonly: _check_readonly(handler)
activate check_readonly
alt [handler.readonly]
check_readonly->>reject_readonly: reject_readonly(handler)
activate reject_readonly
reject_readonly-->>Client: 403 {success: False, error: ...}
deactivate reject_readonly
else [not handler.readonly]
check_readonly-->>PostHandler: proceed
PostHandler-->>Client: state-changing response
end
deactivate check_readonly
deactivate PostHandler
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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
1 participant
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.
Description
Converts the six remaining
unittest.TestCasefiles to plain pytest functions on the shared fixtures.No production code changes, no behaviour changes, no new tests.
py/tests/unit/now has noTestCaseclasses; every remaining one is an HTTP test inintegration/.Fixes : #1695
Motivation and Context
A
TestCasemethod cannot receive a pytest fixture, so each class re-implemented setupconftest.pyalready provides.How Has This Been Tested?
Python 3.12.13,
pytest -m "not server".black --check pyclean under 23.1.0, the version pinned inTypes of changes
Checklist:
py/visdom/VERSIONnot applicable, test-only changeSummary by Sourcery
Refactor tests to use pytest fixtures and expand coverage around server, client, storage, and CI behavior while keeping production functionality unchanged.
Enhancements:
Build:
CI:
Documentation:
Tests: