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

feat!: provide additional selectors to ElementQuery #1774

Merged
merged 1 commit into from
Apr 8, 2024
Merged

Conversation

joelpop
Copy link
Collaborator

@joelpop joelpop commented Mar 22, 2024

Description

Fixes #662 & #1183 (partially)

Adds some selectors to ElementQuery (integration testing) to parallel those provided by ComponentQuery (unit testing). The selectors added in this change are limited to the ones that are "attribute" based.

  • ElementQuery<T> withAttribute(String attribute)
  • ElementQuery<T> withAttribute(String attribute, String value)
  • ElementQuery<T> withAttributeContaining(String attribute, String value)
  • ElementQuery<T> withoutAttribute(String attribute)
  • ElementQuery<T> withoutAttribute(String attribute, String value)
  • ElementQuery<T> withoutAttributeContaining(String attribute, String value)
  • ElementQuery<T> withId(String id)
  • ElementQuery<T> withClassName(String... className)
  • ElementQuery<T> withoutClassName(String... className)
  • ElementQuery<T> withTheme(String theme)
  • ElementQuery<T> withoutTheme(String theme)
  • T single()

In addition, three existing selectors that did not match the naming convention of the ComponentQuery (unit testing) selectors were deprecated in lieu of the new ones.

  • ElementQuery<T> hasAttribute(String name)ElementQuery<T> withAttribute(String attribute)
  • ElementQuery<T> attribute(String name, String value)ElementQuery<T> withAttribute(String attribute, String value)
  • ElementQuery<T> attributeContains(String name, String token)ElementQuery<T> withAttributeContaining(String attribute, String value)

Added both unit and integration tests for the new selectors and for others that were missing.

Updated the README.md file with instructions for configuring and running integration tests in the IntelliJ IDEA IDE.

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

Additional missing selectors are currently being considered, such as:

  • ElementQuery<T> withCondition(Predicate<T> condition)
  • ElementQuery<T> withCaption(String caption)
  • ElementQuery<T> withCaptionContaining(String text)
  • ElementQuery<T> withText(String text)
  • ElementQuery<T> withTextContaining(String text)
  • <V> ElementQuery<T> withValue(V value)

@joelpop joelpop requested review from Artur- and mcollovati March 22, 2024 06:15
@joelpop joelpop self-assigned this Mar 22, 2024
@CLAassistant
Copy link

CLAassistant commented Mar 22, 2024

CLA assistant check
All committers have signed the CLA.

@joelpop
Copy link
Collaborator Author

joelpop commented Mar 23, 2024

@mshabarov

I want to point out one thing I changed in the implementation of ElementQuery::id that deserves some discussion.

Up until now there was no ElementQuery::single method or ElementQuery::withId method. Because of that, ElementQuery::id used ElementQuery::first. However, in one of the test cases (searchShadowDomBeforeLight) there were intentionally two ids to be found, so ElementQuery::first returned just the first one, and all was happy. However, I have implemented ElementQuery::single and ElementQuery::withId and then reimplemented ElementQuery::id to use them

public T id(String id) {
    return withId(id).single();
}

to mirror how ComponentQuery::id is implemented for unit tests. That change subsequently caused the searchShadowDomBeforeLight integration test to fail. I have changed the test from selecting on .id(id) to selecting on .withId(id).first() so its behavior is now as before.

What I want to make sure of is that this is not breaking anything in valid Vaadin light/shadow DOM HTML. It would be simple enough to switch ComponentQuery::id's implementation back to .withId().first(), but that seems wrong, because ids should be unique on a page.

However, this could potentially break some customers' tests, but that might be a valuable discovery for them—that they have duplicate ids.

I have added a note to ComponentQuery::id's Javadoc explaining the new behavior and how to revert to the previous behavior, if desired. My guess is that this change will have extremely low abrasion, and the silver lining is if the change does break something, it will break it for the good.

@joelpop joelpop requested a review from mshabarov March 23, 2024 22:11
Copy link
Contributor

@mcollovati mcollovati left a comment

Choose a reason for hiding this comment

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

Good job and great that you updated the README!
However, once all the comments will be addressed, I propose to squash the change into two separated commits, one for the README update and the other for the code changes.

@mcollovati mcollovati changed the title feature #662 #1183 feat!: provide additional selectors to ElementQuery Apr 3, 2024
@mshabarov
Copy link
Contributor

mshabarov commented Apr 5, 2024

I want to point out one thing I changed in the implementation of ElementQuery::id that deserves some discussion.

First of all, I agree that fail fast if IDs are duplicated is an expected behaviour for ElementQuery::id and also it's better to keep it in sync with ComponentQuery::id, so I think we can change that.

I recommend to update ElementQuery::id javadoc to highlight this and tell how to get an element with duplicated ID if one wants this for any reason. Also, it probably makes sense to recommend the same in the error message that is thrown by ElementQuery::id, if it finds more than one element with the same ID.

@joelpop
Copy link
Collaborator Author

joelpop commented Apr 5, 2024

I want to point out one thing I changed in the implementation of ElementQuery::id that deserves some discussion.

First of all, I agree that fail fast if IDs are duplicated is an expected behaviour for ElementQuery::id and also it's better to keep it in sync with ComponentQuery::id, so I think we can change that.

I recommend to update ElementQuery::id javadoc to highlight this and tell how to get an element with duplicated ID if one wants this for any reason. Also, it probably makes sense to recommend the same in the error message that is thrown by ElementQuery::id, if it finds more than one element with the same ID.

Yes, this commenting has already been done.

Copy link
Contributor

@mcollovati mcollovati left a comment

Choose a reason for hiding this comment

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

LGTM.
I would just squash into two commits, one for README and the other for Java changes before merging into main

@mcollovati mcollovati force-pushed the #1183 branch 2 times, most recently from e4d395d to 9b8bc4b Compare April 8, 2024 14:07
@mcollovati
Copy link
Contributor

@mshabarov it looks like "Rebase and merge" is not enabled on this repo.
I'll create a new PR with the README updates commit only, and once that is merged we can rebase this one on top of main.

@mcollovati mcollovati enabled auto-merge (squash) April 8, 2024 15:36
@mcollovati mcollovati merged commit 3df0337 into main Apr 8, 2024
2 checks passed
@mcollovati mcollovati deleted the #1183 branch April 8, 2024 15:55
mcollovati pushed a commit that referenced this pull request Apr 17, 2024
)

In contrast to ComponentQuerys of unit testing TestBench, ElementQuerys in E2E testing TestBench have (up until now) only provided attribute-based selector methods. This was expanded upon in PR #1774 to provide selector methods for common attributes, such as those for class names, theme, and id. This PR adds condition-based selector methods to ElementQuery to mirror and expand upon those of ComponentQuery. Namely:

withCondition
withPropertyValue[Containing]
withLabel[Containing]
withPlaceholder[Containing]
withCaption[Containing]
withText[Containing]

Elements having their labels in their text (such as buttons) must implement a new, method-less interface HasLabelAsText in their element tester (such as NativeButtonElement for the NativeButton component and ButtonElement for the Button component) in order for the withCaption[Containing] selectors to work for them.

This PR brings the E2E testing selectors up to par with the unit testing selectors. The only one missing is withValue.

Fixes #1183
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ElementQuery should support finding by class
4 participants