test: cover client plot and media payloads, fix images() tile padding (PyTest - 9) - #1687
Open
Manik-Khajuria-5 wants to merge 47 commits into
Open
test: cover client plot and media payloads, fix images() tile padding (PyTest - 9)#1687Manik-Khajuria-5 wants to merge 47 commits into
Manik-Khajuria-5 wants to merge 47 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
Contributor
Reviewer's GuideAdds extensive unit and integration coverage for client Plotly/media payloads, server env/storage durability, socket and HTTP behavior, and refines multiple server utilities (atomic JSONStore writes, readonly enforcement, safer updates, auth hardening, plot/image history caps, and images() padding), while restructuring the test suite layout and pytest configuration. 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)
check_auth-->>PostHandler: auth ok
PostHandler->>check_readonly: _check_readonly(handler)
alt handler.readonly
check_readonly->>reject_readonly: reject_readonly(handler)
reject_readonly-->>Client: 403 {success: False, error: ...}
else not readonly
check_readonly-->>PostHandler: proceed
PostHandler-->>Client: 200 response
end
deactivate PostHandler
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
Unit tests for the client methods that build the Plotly payload sent to the browser:
client_payloads_shapes.py(54)quiver,stem,pie,mesh,dual_axis_linesclient_payloads_graph.py(77)sunburst,sankey,graph,violin,parallel_coordinatesclient_payloads_media.py(85)image,images,image_heatmap,image_select,audio,video,svgSuite total: 1,428 tests (was 1,212).
Fixes : #1695
Motivation and Context
A wrong key or a broken scaling branch here raises nothing in Python the plot just renders wrong. The tests decode the payload (base64 images back into arrays) and assert the real contract with the frontend.
How Has This Been Tested?
blackclean; `pTypes of changes
Checklist:
py/visdom/VERSIONaccording to Semantic VersioningSummary by Sourcery
Add extensive unit and integration test coverage for client plotting/media payloads, server env/storage/sockets behavior, and CLI/build utilities, while hardening env persistence, readonly/write paths, authentication, and plot/update edge cases.
Bug Fixes:
Enhancements:
Build:
Tests: