[e2e-tests] Add partial suite of tests for 'upload' feature specification - #4929
Conversation
jonathonherbert
changed the title
781a8bc to
da74f89
Compare
jonathonherbert
changed the title
| import type { Page } from '@playwright/test'; | ||
|
|
||
| declare global { | ||
| interface Window { |
There was a problem hiding this comment.
Perhaps this could come from the application definition 🤔
|
|
||
| @todo | ||
| Scenario: Metadata editing is disabled without edit permission | ||
| Given I am not permitted to edit the image |
There was a problem hiding this comment.
On an Upload page, this situation can only occur if user without edit_metadata permission uploads an image previously uploaded by someone else. Everyone is allowed to edit metadata for any images genuinely uploaded by them, even if they have no edit_metadata permission to edit uploads made by other users.
No idea how useful is it to review those scenarios. But somehow it’s very, very, cool to be reading them!
There was a problem hiding this comment.
Good shout, I've updated the description to look like Given I am not permitted to edit the image, as it has been uploaded by another user and I do not have edit_metadata permission in 208587b
e4e2f39 to
465256b
Compare
…r an image I did not upload cc. @paperboyo @ #4929 (comment)
da74f89 to
b4eb729
Compare
…r an image I did not upload cc. @paperboyo @ #4929 (comment)
b4eb729 to
73971fd
Compare
…r an image I did not upload cc. @paperboyo @ #4929 (comment)
cb6b403 to
f11c22f
Compare
The upload feature file specifies far more than we are wiring up in the first pass. Tag everything we are not implementing yet with @todo and exclude those tags from bddgen, so its "undefined step" errors stay meaningful for the scenarios we do implement. Gherkin tags cannot contain whitespace, so the pre-existing @todo annotations were failing to parse; their notes move to comments. Uploads mutate the single shared e2e user's upload history, so the feature runs @mode:serial.
Wires up the upload page shell, prompt and select-files scenarios against the
real stack: eight scenarios covering the upload tools rendering, navigation to
search and to a filtered search, the current-uploads section, the prompt copy,
the file picker, queueing selected files, and the upload size limit.
Locators live in one place (steps/support/upload-page.ts) and prefer roles and
accessible names. `<dnd-uploader>` gains a region role so "the drag-and-drop
uploader should be active" has a hook.
Two things needed handling to make the page testable at all:
* Kahuna's first-run graphic-image blur explainer covers the top bar, so the
background accepts the default via its cookie.
* An upload reaches a terminal state within about a second, so the scenario
that needs one in flight holds the transfer to the ingest bucket open.
Two more scenarios are tagged @todo because the app cannot currently satisfy
them, with the reason recorded against each: the "uploads in progress" leave
confirmation never fires (ui-router destroys the controller scope before
$locationChangeStart), and the prompt's example label is guarded by a `ctrl`
that does not exist in filePrompt's isolated scope.
The warning does fire, but only for history navigations. On a ui-sref click ui-router destroys the UploadCtrl scope before broadcasting $locationChangeStart, so the listener registered on that scope never runs; pressing back reaches it. The step now goes back rather than clicking the top bar link. For back to have somewhere to return to within the same document, the background arrives at the upload page from search the way a user does, instead of deep-linking to /upload.
prompt.html guarded the hint with `ctrl.presetLabels` and `ctrl.active`, neither of which filePrompt provided. That name was not simply unset: the nested <file-uploader> has no scope of its own, so its `controllerAs: 'ctrl'` publishes FileUploaderCtrl onto the prompt's scope, and the guard read properties off that. Expose the check on the prompt's own scope instead, sourced from presetLabelService so it follows labels being added and removed in gr-preset-labels' isolated scope. The `!active` part of the guard is dropped: while the add-label form is open its input already shows the same example as a placeholder.
73971fd to
e94a877
Compare
|
Seen on auth, usage, image-loader, metadata-editor, thrall, leases, media-api, kahuna (merged by @jonathonherbert 10 minutes and 6 seconds ago) Please check your changes! |
|
Seen on cropper (merged by @jonathonherbert 10 minutes and 41 seconds ago) Please check your changes! |
gu-prout
Bot
added
Seen-on-cropper
Seen-on-collections
Pending-on-cropper
and removed
Pending-on-cropper
Pending-on-collections
Seen-on-cropper
labels
|
Seen on collections (merged by @jonathonherbert 10 minutes and 41 seconds ago) Please check your changes! |
|
Seen on collections, cropper (merged by @jonathonherbert 10 minutes and 41 seconds ago) Please check your changes! |
|
Seen on collections (merged by @jonathonherbert 10 minutes and 41 seconds ago) Please check your changes! |
|
Seen on collections, cropper (merged by @jonathonherbert 10 minutes and 44 seconds ago) Please check your changes! |
1 similar comment
|
Seen on collections, cropper (merged by @jonathonherbert 10 minutes and 44 seconds ago) Please check your changes! |
co-authored-by: @claude (Opus 5) — words are mine.
What does this change?
Adds the first batch of end-to-end tests for the upload feature specified in #4900. The specs included are:
Notes
I felt that breaking the suite into chunks would make it easier to review, and there are also some changes to make to ensure later parts of the spec pass, including provisioning users in cookies for permissions, and fixing bugs, which are probably best done in specific PRs.
Nevertheless, this is a marker for the general approach, and gives us an idea of how the suite might perform as it grows to encompass the application generally. At the moment these tests are run serially, as some of them perform writes and we will run into concurrency issues running them in parallel. It may be useful to split the suite as it grows to permit parallel execution — the operation mode can only be set at the feature level (docs).
Encouragingly, the extra scaffolding in
setup.tsseems fairly thin, and the approach seems sensible to me, but comments on how this looks from others with adjacent experience (@dominickendrick @waisingyiu) would be much appreciated, if only from the point of view of standardising across projects.Who should look at this?
Tested? Documented?