test : Cover the client's metric and layout helpers, and apply a lone tick step (PyTest - 8) - #1683
Open
Manik-Khajuria-5 wants to merge 45 commits into
Open
test : Cover the client's metric and layout helpers, and apply a lone tick step (PyTest - 8)#1683Manik-Khajuria-5 wants to merge 45 commits into
Manik-Khajuria-5 wants to merge 45 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 4, 2026 09:51
Contributor
Reviewer's GuideAdds extensive unit and integration tests for client helpers, metrics, server handlers, sockets, storage wiring, and t-SNE, while fixing multiple robustness and security issues (atomic JSONStore writes, label normalization warnings, readonly enforcement, authentication timing, layout broadcast, plot/image history caps, and error-page behavior). Sequence diagram for readonly enforcement on HTTP handlerssequenceDiagram
actor Client
participant UpdateHandler as UpdateHandler.post
participant check_readonly
participant reject_readonly
Client->>UpdateHandler: post(request)
UpdateHandler->>check_readonly: check_readonly(handler)
alt handler.readonly is True
check_readonly->>reject_readonly: reject_readonly(handler)
reject_readonly->>Client: 403 {success: False, error: "The server is running in readonly mode"}
else handler.readonly is False
check_readonly->>UpdateHandler: call original post(handler)
UpdateHandler-->>Client: 200 OK (state changed)
end
Sequence diagram for capped plot_history updatessequenceDiagram
actor Client
participant UpdateHandler as UpdateHandler
participant UpdatePacket as UpdateHandler.update_packet
participant Update as UpdateHandler.update
Client->>UpdateHandler: post(update packet)
UpdateHandler->>UpdatePacket: update_packet(p, args, max_text_lines, max_old_content, max_image_history, max_plot_history)
UpdatePacket->>Update: update(p, args, max_text_lines, max_old_content, max_image_history, max_plot_history)
Update->>Update: [p.type == "plot"]
Update->>Update: [args.data[0].type == "plot_history"]
Update->>Update: p.content.append(args.data[0].content)
alt len(p.content) > max_plot_history
Update->>Update: p.content = p.content[-max_plot_history:]
end
Update-->>UpdatePacket: updated packet p
UpdatePacket-->>UpdateHandler: updated packet p
UpdateHandler-->>Client: 200 OK (plot history updated)
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 unit coverage for the private helpers at the top of
py/visdom/__init__.py, and fixes one defect found while writing it.py/tests/unit/client_metrics.py(113 tests) the curve/metric helpers behindroc_curve,pr_curveandconfusion_matrix, plus a payload round trip over those three public methods.py/tests/unit/client_helpers.py(153 tests)_axisformat,_opts2layout,_assert_opts,_normalize_labels, the marker/line validators and_decode_binary_arrays._axisformatnow appliesopts.xtickstepwhen it is the only axis option given._normalize_labelsno longer emits a misleading numpyRuntimeWarning.Fixes : #1695
Motivation and Context
These helpers build the Plotly layout and compute the numbers users read off ROC/PR curves, but nothing in
py/tests/touched them. A regression here shows up as a subtly wrong plot rather than a failure.How Has This Been Tested?
python -Ogreen on both new files;blackclean.fails exactly its own regression tests and nothing else.
scatter(opts=dict(xtickstep=0.5))now storesxaxis: {"dtick": 0.5}; the axis was absent from the layout befoTypes of changes
Checklist:
py/visdom/VERSIONSummary by Sourcery
Strengthen visdom's reliability and safety by hardening environment persistence, update and comparison logic, readonly and auth enforcement, socket and polling behavior, and by significantly expanding unit and integration test coverage alongside test infrastructure and packaging configuration updates.
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: