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

add cachelookup annotation support #1894

Merged
merged 3 commits into from
Aug 3, 2022

Conversation

groov1kk
Copy link
Contributor

@groov1kk groov1kk commented Jul 14, 2022

Proposed changes

Added support of @CachedLookup selenium annotation for page objects:

public class SelectsPage {

    @CacheLookup
    @FindBy(xpath = "//select[@name='domain']")
    WebElement domainSelect;

    @CacheLookup
    @FindBy(tagName = "h1")
    SelenideElement h1;

    @CacheLookup
    @FindBy(tagName = "h2")
    List<SelenideElement> h2s;

    @CacheLookup
    @FindBy(tagName = "h2")
    ElementsCollection h2sElementsCollection;

    @CacheLookup
    @FindBy(id = "status")
    StatusBlock status;

    @CacheLookup
    @FindBy(css = "#user-table tbody tr")
    List<UserInfo> userInfoList;

  }

Checklist

  • Checkstyle and unit tests are passed locally with my changes by running gradlew check chrome_headless firefox_headless command
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@groov1kk groov1kk force-pushed the cachelookup-annotation-support branch from 1d0d7f5 to 3a13663 Compare July 14, 2022 21:02
@asolntsev asolntsev self-assigned this Jul 17, 2022
@asolntsev
Copy link
Member

Hi @groov1kk !
I like how you implemented this feature. A deep understanding of the existing code base. Nice!

I have only two concerns:

  1. Is it really needed? Did you have a chance to measure how many seconds will this feature save?
    When I measured it last time, the gain for so small that I decided element caching is not worth the effort. It gave so few seconds... Most of the times, the slowness comes from browser, backend, services etc., but not from finding the web elements.
  2. If we decide it's needed, it would be also great to implement caching of elements in the "standard" methods $ and $$. But what would be the API? Should it be a global setting, or an additional parameter in overloaded $/$$ method?

@groov1kk groov1kk force-pushed the cachelookup-annotation-support branch from 3a13663 to 347d1a0 Compare July 18, 2022 17:22
@sonarcloud
Copy link

sonarcloud bot commented Jul 18, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@groov1kk
Copy link
Contributor Author

groov1kk commented Jul 18, 2022

Hello

  1. From my experience, I received benefit from this feature when I worked with complex static structures such as tables, in cases when I was needed to iterate through many rows and cells to find desired element. In such structures you couldn't collect all elements using single findElements request and have to make additional sub-finds. And, for example, I had to iterate through this structure several times in one test. But yes, it wasn't significant improvement, something about 5-15 seconds. Anyway, pure selenium has this feature and I just have a habbit to mark all static content with this annotation just not to make additional useless requests :)
  2. I thought about this. It shouldn't be a global feature because usually we have to work both with static and dynamic elements. Developer must decide how to work with elements on the spot. So, either we can overload existing methods ($(cssSelector, shouldCache), etc) or create different methods (cache(seleniumSelector), cacheAll(cssSelector), xCache(xpathSelector), etc)

@asolntsev asolntsev added this to the 6.7.0 milestone Aug 2, 2022
@asolntsev asolntsev removed this from the 6.7.0 milestone Aug 2, 2022
@asolntsev asolntsev added this to the 6.7.0 milestone Aug 3, 2022
@asolntsev asolntsev self-requested a review August 3, 2022 06:40
Copy link
Member

@asolntsev asolntsev left a comment

Choose a reason for hiding this comment

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

Seems a very well done PR to me.
Thumbs up! @groov1kk

@asolntsev asolntsev merged commit cf0714f into selenide:master Aug 3, 2022
asolntsev added a commit that referenced this pull request Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants