Skip to content

Add sudo mode acceptance tests, per #8210 feedback #8237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 5, 2024

Conversation

LawnGnome
Copy link
Contributor

Whichever stage of sudo grief we're at, we're now at acceptance.

I don't think there's anything surprising here. I used the yank button as a proxy for the general PrivilegedAction functionality rather than creating a new test-only component, since it's already on main and is unlikely to change significantly. Unit tests are covering all the actually interesting bits anyway.

The one thing that I was surprised by is that the qunit-dom helpers were capable of clicking a button in a disabled fieldset, even though that's not possible from the browser UI, which means I can't write a sensible "failed to yank" style test. (I tried a few different ways just to be sure.)

@LawnGnome LawnGnome requested a review from Turbo87 March 4, 2024 23:53
@Turbo87
Copy link
Member

Turbo87 commented Mar 5, 2024

The one thing that I was surprised by is that the qunit-dom helpers were capable of clicking a button in a disabled fieldset

The click() fn comes from https://github.com/emberjs/ember-test-helpers, not qunit-dom, which only deals with DOM state assertions.

https://github.com/emberjs/ember-test-helpers/blob/master/addon/addon-test-support/%40ember/test-helpers/dom/click.ts is the implementation of click(), which emulates the native events that the browser would fire when a users clicks a button. But admittedly https://github.com/emberjs/ember-test-helpers/blob/9cec68dc6aa9c0a7a449eb89797eb81299fa727f/addon/addon-test-support/%40ember/test-helpers/dom/click.ts#L116-L118 only checks if the element itself is disabled, but does not care about parent fieldsets or forms.

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

we might want an additional test though that checks that the sudo mode controls are invisible when the user is not an admin

@Turbo87 Turbo87 added A-frontend 🐹 C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear labels Mar 5, 2024
@LawnGnome
Copy link
Contributor Author

we might want an additional test though that checks that the sudo mode controls are invisible when the user is not an admin

Good call; added.

@LawnGnome LawnGnome merged commit 213b184 into rust-lang:main Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend 🐹 C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants