test : Cover environment comparison, environment transfer and the server CLI with tests (PyTest - 7) - #1682
Open
Manik-Khajuria-5 wants to merge 41 commits into
Open
test : Cover environment comparison, environment transfer and the server CLI with tests (PyTest - 7)#1682Manik-Khajuria-5 wants to merge 41 commits into
Manik-Khajuria-5 wants to merge 41 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.
A python client running with use_polling=True POSTs to /vis_socket_wrap with no sid to get one minted. That path built a VisSocketWrapper and initialized it without ever assigning .request, so open() raised AttributeError reaching for request.remote_ip and the endpoint answered 500 -- polling mode could not connect at all. The subscriber route a few lines below already assigns it; do the same here.
Polling had no tests at all, while AGENTS.md asks for every socket feature to work over both transports. Covers minting a sid on each route, the handshake, the three protocol failures, ten commands driven end to end over HTTP, the readonly short-circuit, the idle reaper and the pending-message deque. Two behaviours are pinned rather than changed: a polling subscriber receives a fourth, redundant layout_update because it registers before initialize broadcasts, and the deque is unbounded, which is what makes the reaper load-bearing.
The submitted username and derived key were checked with ==, which short-circuits on the first differing character. That leaks, through response timing, how much of a guess was correct, and the short-circuit between the two checks leaked whether the username alone was right. hmac.compare_digest on both halves removes the signal.
-readonly stopped socket commands and blocked uploads and experiment logging, but the HTTP routes that create, update, close, fork, delete and save were left open, so any client could still mutate a server started readonly. Adds a check_readonly decorator next to check_auth and puts it on those six endpoints, plus the one write hidden behind /win_data, which also serves reads and so checks inline. readonly moves onto _WEB_APP_ATTRIBUTES, which is where every other per-request attribute already comes from; the two handlers that copied it by hand no longer need an initialize override.
Neither gate had any tests. Covers the login page and its form, the four ways a credential can be wrong, 401 on every authenticated route with the state left untouched, the same routes succeeding with the issued cookie, a forged cookie, /health staying public by design, and both socket kinds closing instead of registering when unauthenticated. The readonly half asserts 403 and no state change for each of the nine mutating routes, that reads and the env page still work, and that an ordinary server still answers 200 on all of them -- otherwise a decorator that refused everything would look like a pass.
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.
![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,
rajnisht7,
tonypzy and
vedansh-5
August 3, 2026 11:36
Contributor
Reviewer's GuideAdds extensive unit and integration coverage around environment persistence, socket/HTTP command handling, and the server CLI, plus a small robustness fix in compare_envs and atomic JSONStore writes. Sequence diagram for readonly HTTP write handlingsequenceDiagram
actor Client
participant UpdateHandler as UpdateHandler.post
participant check_auth
participant check_readonly
participant reject_readonly
participant JSONStore
Client->>UpdateHandler: POST /update
UpdateHandler->>check_auth: _check_auth(handler)
check_auth-->>UpdateHandler: authenticated
UpdateHandler->>check_readonly: _check_readonly(handler)
alt handler.readonly is True
check_readonly->>reject_readonly: reject_readonly(handler)
reject_readonly-->>Client: 403 readonly response
else handler.readonly is False
check_readonly-->>UpdateHandler: call wrapped post
UpdateHandler->>JSONStore: save_env(eid, state[eid])
JSONStore-->>UpdateHandler: success
UpdateHandler-->>Client: 200 OK
end
File-Level Changes
Possibly linked issues
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
Adds three test modules and one robustness fix they uncovered.
py/tests/unit/compare_envs.py:server_utils.compare_envs: title-based merging, env numbering/legend, image panes.py/tests/integration/env_transfer.py:/upload_env,/fork_env,/save,/env_state,/win_datainput handling.py/tests/unit/run_server_cli.py:run_server.main(port validation,base_url, flags, env credentials, SSL pairing) andbuild.download_scripts(URL table, subdirectory routing,version.builtstamp).compare_envsnow skips a pane when the base window has nocontentor its plot data is empty.139 tests; no production behaviour change beyond that guard.
Fixes : #1695
Integration note
This PR should be merged after #1679 , as it depends on the lower-layer changes introduced there.
Motivation and Context
These three areas had no coverage and are awkward to exercise by hand:
main()ends in a blocking IO loop,download_scripts()reaches three CDNs, andcompare_envswrites to a socket instead of returningTypes of changes
Checklist:
py/visdom/VERSIONaccordhttps://semver.org/) — not needed, tests plus an internal guardSummary by Sourcery
Expand and restructure the visdom test suite to cover environment storage durability, server/window/env lifecycle, socket and polling behaviour, auth/readonly enforcement, and CLI/build utilities, while hardening core server utilities and handler logic for robustness and readonly safety.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests:
Chores: