vaadin / testbench

Vaadin TestBench is a tool for automated user interface testing of Vaadin applications.
https://vaadin.com/testbench
Other
20 stars 22 forks source link

feat!: provide additional selectors to ElementQuery #1774

Closed joelpop closed 2 months ago

joelpop commented 3 months ago

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.

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.

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

Checklist

Additional for Feature type of change

Additional missing selectors are currently being considered, such as:

CLAassistant commented 3 months ago

CLA assistant check
All committers have signed the CLA.

joelpop commented 3 months ago

@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.

mshabarov commented 3 months ago

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 commented 3 months ago

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.

mcollovati commented 2 months ago

@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.