Skip to content
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

Support UserAttributes in to all components #1843

Closed
huysentruitw opened this issue Jun 10, 2021 · 17 comments · Fixed by #1848
Closed

Support UserAttributes in to all components #1843

huysentruitw opened this issue Jun 10, 2021 · 17 comments · Fixed by #1848
Assignees
Labels
enhancement New feature or request

Comments

@huysentruitw
Copy link
Contributor

huysentruitw commented Jun 10, 2021

I want to start testing a lot of components in my application with bUnit. A pattern I'm using (and a lot of others are using) for selecting elements is to use the data-testid attribute on elements you want to select instead of using a more complex query-selector. See also this post by Kent C. Dodds. It's also a best practice listed in the cypress docs

Proposal

My proposal would be to add a parameter TestId to MudComponentBase and to use that parameter to set the data-testid attribute on the first element of each component. I am open to create a PR for this.

Any other ideas are welcome, my example would be this, how to select the increment or decrement button giving this template:

<MudFab Color="Color.Secondary" Icon="@Icons.Filled.Remove" IconColor="Color.Primary" 
            Size="Size.Large" IconSize="Size.Large" DisableElevation="true" OnClick="Decrement"
            Disabled="Value <= MinimumValue || Disabled"/>
<MudFab Color="Color.Secondary" Icon="@Icons.Filled.Add" IconColor="Color.Primary"
            Size="Size.Large" IconSize="Size.Large" DisableElevation="true" OnClick="Increment"
            Disabled="Value >= MaximumValue || Disabled"/>

Alternatives I've considered

The alternative would be to use UserAttributes, but these are not implemented for all components and are also a lot more cumbersome to set (as we need to pass a dictionary).

@huysentruitw huysentruitw added enhancement New feature or request triage labels Jun 10, 2021
@mikes-gh
Copy link
Contributor

mikes-gh commented Jun 10, 2021

So we would need a data-testid attribute on all elements that are selected in tests?
I can see this separates the styling from the test selection but that sounds like a huge change.
Our styling classes are well named at the moment and shouldn't change hopefully.

@huysentruitw
Copy link
Contributor Author

huysentruitw commented Jun 10, 2021

What would you suggest if there are multiple elements of the same type (buttons as in my example)? I'm currently adding a dummy class that I can use as selector, but it's a bit weird that there's no CSS for those classes.

@porkopek
Copy link
Contributor

We already provide a Tag property in MudComponentBase. It's of type object, so you can attach whatever you want, and it has no other purposes. We never set it, so the user can do it.

Could it be enough for your purposes?

@porkopek
Copy link
Contributor

I think I understood better now, and you want we pass that to every first element in the component.

Why not simply use .FindAll and you have an array and pick them by order?

@henon
Copy link
Collaborator

henon commented Jun 10, 2021

The alternative would be to use UserAttributes, but these are not implemented for all components and are also a lot more cumbersome to set (as we need to pass a dictionary).

You don't need a dictionary for UserAttributes. You can directly set them in the markup like this:

<MudButton data-testid="myButton"> ... </MudButton>

For the components who don't support them we'll just do so. It would be trivial for you to PR that.

@porkopek
Copy link
Contributor

The alternative would be to use UserAttributes, but these are not implemented for all components and are also a lot more cumbersome to set (as we need to pass a dictionary).

You don't need a dictionary for UserAttributes. You can directly set them in the markup like this:

<MudButton data-testid="myButton"> ... </MudButton>

For the components who don't support them we'll just do so. It would be trivial for you to PR that.

Is there a guaranty that the first element will receive the UserAttributes?
I can't remember how @JonBunator managed this

@huysentruitw
Copy link
Contributor Author

Ah, didn't know usage was so easy, that would definitely help for me. I will create a PR to add UserAttributes where it's missing.

@henon
Copy link
Collaborator

henon commented Jun 10, 2021

Is there a guaranty that the first element will receive the UserAttributes?

We have total control where the user attributes are applied. Look for @attributes="UserAttributes" in razor

@porkopek
Copy link
Contributor

I know, but sometimes they are applied to the first element, others to the more convenient element.
I think @JonBunator changed that to uniformize it and be more consistent

@huysentruitw
Copy link
Contributor Author

Yes, sometimes userattributes is passed to an underlying input element, which isn't really a problem for testing, as long as it's consistent.

I'm currently writing a test that uses reflection to get all MudComponentBase derivates and I then plan to use bunit to see if UserAttributes gets passed to an underlying element. Once we have this test, it would be impossible to forget about it when creating new components in future. But first, dinner 🍽

@huysentruitw
Copy link
Contributor Author

I think I understood better now, and you want we pass that to every first element in the component.

Why not simply use .FindAll and you have an array and pick them by order?

Btw trusting on order is very error prone in UI tests. Specifically in this Up/Down component, the customer has already requested to swap the up/down buttons twice 😁 Changes like this may break the snapshot test but should not break any functional test.

@huysentruitw
Copy link
Contributor Author

huysentruitw commented Jun 10, 2021

Test is starting to work:

image

Still have to get a trick out of my sleeve for generic components 😬

@henon henon removed the triage label Jun 10, 2021
@henon henon added this to Triage (decision to be made) in Feature requests via automation Jun 10, 2021
@henon henon moved this from Triage (decision to be made) to Under development in Feature requests Jun 10, 2021
@henon henon changed the title Add TestId to all components Support UserAttributes in to all components Jun 10, 2021
@JonBunator
Copy link
Member

Is there a guaranty that the first element will receive the UserAttributes?
I can't remember how @JonBunator managed this

I only added missing Class and Style properties in my PR. I didn't know about the existence of the UserAttributes property at the time. Writing a bunit test which tests all components using reflection is such a great idea. Why didn't I come up with this haha.

@mikes-gh
Copy link
Contributor

What about time picker for example or date picker. We test against many different sub-elements? We also loop through element arrays to test.

@huysentruitw
Copy link
Contributor Author

O.M.G. This is way more work than I was thinking. I'm trying to get to the point where the test works, and we can improve from there.

@huysentruitw
Copy link
Contributor Author

huysentruitw commented Jun 10, 2021

I have created a PR, which automatically tests all components for the forwarding of UserAttributes. However, you'll see that I had exclude a few components from my test because I don't know how to create them properly. Feel free to take a look at it. Meanwhile, I'm hungry again 😀

@henon henon linked a pull request Jun 11, 2021 that will close this issue
@huysentruitw
Copy link
Contributor Author

huysentruitw commented Jun 11, 2021

My PR is finished. Now I only have to exclude 4 components, which are base components that we shouldn't test.

I also did a huge refactor and separated the component creation from the test itself, making things readable again (at least from my POV 😉). The MudComponentFactory could be extended to also add Style and Class parameters and reuse it for similar Style/Class testing.

henon added a commit that referenced this issue Jun 12, 2021
* Use UserAttributes in all components + enforce with test

* Automatically generate generic types using string as TData

* Remove dead comment

* Update comment

* Change old-style code-behind for MudMenuItem

* Change old-style code-behind for MudLegend

* Test MudElement

* Test MudSelectItem

* Test MudTabs

* Test MudTabPanel

* Update comments

* Test MudDialog

* Test MudMessageBox

* Refactor

* Finish up :)

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
@henon henon removed this from Under development in Feature requests Jun 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants