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

#623: Add methods to get first and last n elements of the collection #624

Merged
merged 2 commits into from Oct 23, 2017

Conversation

Projects
None yet
5 participants
@ostap-oleksyn
Contributor

ostap-oleksyn commented Oct 22, 2017

Proposed changes

As per this request, I added first() and last() methods to get the first and last n elements of the elements list.

Checklist

  • [x ] Checkstyle and unit tests pass locally with my changes by running gradle check chrome htmlunit command
  • [x ] I have added tests that prove my fix is effective or that my feature works
  • [x ] I have added necessary documentation (if appropriate)
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 22, 2017

Coverage Status

Coverage decreased (-0.09%) to 64.007% when pulling fa0ca66 on ostap-oleksyn:first_last_collection into bec8497 on codeborne:master.

coveralls commented Oct 22, 2017

Coverage Status

Coverage decreased (-0.09%) to 64.007% when pulling fa0ca66 on ostap-oleksyn:first_last_collection into bec8497 on codeborne:master.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 22, 2017

Codecov Report

Merging #624 into master will decrease coverage by 0.08%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #624      +/-   ##
============================================
- Coverage      60.3%   60.21%   -0.09%     
- Complexity      767      768       +1     
============================================
  Files           148      148              
  Lines          2741     2745       +4     
  Branches        269      269              
============================================
  Hits           1653     1653              
- Misses          984      988       +4     
  Partials        104      104
Impacted Files Coverage Δ Complexity Δ
...ava/com/codeborne/selenide/ElementsCollection.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...e/selenide/impl/WebDriverThreadLocalContainer.java 79.71% <0%> (-0.73%) 30% <0%> (ø)
...ne/selenide/impl/WebElementsCollectionWrapper.java 100% <0%> (+16.66%) 3% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bec8497...fa0ca66. Read the comment docs.

codecov-io commented Oct 22, 2017

Codecov Report

Merging #624 into master will decrease coverage by 0.08%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #624      +/-   ##
============================================
- Coverage      60.3%   60.21%   -0.09%     
- Complexity      767      768       +1     
============================================
  Files           148      148              
  Lines          2741     2745       +4     
  Branches        269      269              
============================================
  Hits           1653     1653              
- Misses          984      988       +4     
  Partials        104      104
Impacted Files Coverage Δ Complexity Δ
...ava/com/codeborne/selenide/ElementsCollection.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...e/selenide/impl/WebDriverThreadLocalContainer.java 79.71% <0%> (-0.73%) 30% <0%> (ø)
...ne/selenide/impl/WebElementsCollectionWrapper.java 100% <0%> (+16.66%) 3% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bec8497...fa0ca66. Read the comment docs.

@asolntsev asolntsev self-requested a review Oct 23, 2017

@asolntsev asolntsev self-assigned this Oct 23, 2017

@asolntsev asolntsev added this to the 4.9 milestone Oct 23, 2017

@asolntsev asolntsev merged commit fb3d1ee into selenide:master Oct 23, 2017

1 of 4 checks passed

codecov/patch 0% of diff hit (target 60.3%)
Details
codecov/project 60.21% (-0.09%) compared to bec8497
Details
coverage/coveralls Coverage decreased (-0.09%) to 64.007%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ostap-oleksyn ostap-oleksyn deleted the ostap-oleksyn:first_last_collection branch Oct 24, 2017

public void canGetLastNElements() {
ElementsCollection collection = $$x("//select[@name='domain']/option");
collection.last(2).shouldHaveSize(2);
collection.last(10).shouldHaveSize(collection.size());

This comment has been minimized.

@yashaka

yashaka Oct 25, 2017

Contributor

The expected result as a part of test logic should be explicit.
I would change the corresponding line to:
collection.last(10).shouldHaveSize(10);

@yashaka

yashaka Oct 25, 2017

Contributor

The expected result as a part of test logic should be explicit.
I would change the corresponding line to:
collection.last(10).shouldHaveSize(10);

This comment has been minimized.

@ostap-oleksyn

ostap-oleksyn Oct 25, 2017

Contributor

The size of the collection in this case is 4. If you try to get elements that are out of array boundaries the method will just return the full collection instead of throwing an error. That's what that line is testing.

@ostap-oleksyn

ostap-oleksyn Oct 25, 2017

Contributor

The size of the collection in this case is 4. If you try to get elements that are out of array boundaries the method will just return the full collection instead of throwing an error. That's what that line is testing.

This comment has been minimized.

@yashaka

yashaka Oct 27, 2017

Contributor

ah, good, then you should put 4 explicitly in the assert, not collection.size()

@yashaka

yashaka Oct 27, 2017

Contributor

ah, good, then you should put 4 explicitly in the assert, not collection.size()

.map(SelenideElement::getText)
.collect(Collectors.toList());
Assert.assertEquals(regularSublist, selenideSublist);

This comment has been minimized.

@yashaka

yashaka Oct 25, 2017

Contributor

Yet the test logic is not explicit in the test... I recommend to explicitly verify what texts should be in the sub-collection.

@yashaka

yashaka Oct 25, 2017

Contributor

Yet the test logic is not explicit in the test... I recommend to explicitly verify what texts should be in the sub-collection.

.map(SelenideElement::getText)
.collect(Collectors.toList());
Assert.assertEquals(regularSublist, selenideSublist);

This comment has been minimized.

@yashaka

yashaka Oct 25, 2017

Contributor

In addition - just a few tests are not enough. It does make sense to check error messages for such "sub-collections"

@yashaka

yashaka Oct 25, 2017

Contributor

In addition - just a few tests are not enough. It does make sense to check error messages for such "sub-collections"

This comment has been minimized.

@ostap-oleksyn

ostap-oleksyn Oct 25, 2017

Contributor

What error messages do you have in mind?

@ostap-oleksyn

ostap-oleksyn Oct 25, 2017

Contributor

What error messages do you have in mind?

This comment has been minimized.

@yashaka

yashaka Oct 27, 2017

Contributor

In Selenide we have kind of "informative error messages" in cases when the "should" method failed, or some operation on collection element failed. In such cases the message should be really informative. The user should understand from the error message what was the collection that he tried to deal with.

Just run:

$$(".item").last(3).shouldHaveSize(4)
And check the error message. It should be informative. But probably it will not:)
And we should have tests for such cases too.

@yashaka

yashaka Oct 27, 2017

Contributor

In Selenide we have kind of "informative error messages" in cases when the "should" method failed, or some operation on collection element failed. In such cases the message should be really informative. The user should understand from the error message what was the collection that he tried to deal with.

Just run:

$$(".item").last(3).shouldHaveSize(4)
And check the error message. It should be informative. But probably it will not:)
And we should have tests for such cases too.

*/
public ElementsCollection first(int elements) {
List<WebElement> sublist = getActualElements().subList(0, Math.min(elements, size()));
return new ElementsCollection(new WebElementsCollectionWrapper(sublist));

This comment has been minimized.

@yashaka

yashaka Oct 25, 2017

Contributor

This implementation is not correct. It's not "reactive" enough.

GIVEN "1", "2", "c", "d" texts in the '.item' list during first 1 second
WHEN texts change to "a", "b", "c", "d" during next 5 seconds
THEN the code $$(".item").first(2).shouldHave(texts("a", "b")) still will fail on timeout of 6 seconds
BUT it should pass.

This test by the way should also be added to the test suite.

The correct way to implement these methods is the same as for "filterBy" method.

@yashaka

yashaka Oct 25, 2017

Contributor

This implementation is not correct. It's not "reactive" enough.

GIVEN "1", "2", "c", "d" texts in the '.item' list during first 1 second
WHEN texts change to "a", "b", "c", "d" during next 5 seconds
THEN the code $$(".item").first(2).shouldHave(texts("a", "b")) still will fail on timeout of 6 seconds
BUT it should pass.

This test by the way should also be added to the test suite.

The correct way to implement these methods is the same as for "filterBy" method.

This comment has been minimized.

@ostap-oleksyn

ostap-oleksyn Oct 25, 2017

Contributor

It looks like timeout after 6 seconds for collections is the default selenide behavior. Using your example, lets say we have one element with text "1" which changes text to "a" after 5 seconds. If you call $$(".item").shouldHave(texts("a")) without my first()/last() method, it will still fail after 6 seconds. Even if you set the Configuration.timeout to any other value that line still fails after 6 seconds timeout.
This line works fine: $(".item").shouldHave(text("a"))

@ostap-oleksyn

ostap-oleksyn Oct 25, 2017

Contributor

It looks like timeout after 6 seconds for collections is the default selenide behavior. Using your example, lets say we have one element with text "1" which changes text to "a" after 5 seconds. If you call $$(".item").shouldHave(texts("a")) without my first()/last() method, it will still fail after 6 seconds. Even if you set the Configuration.timeout to any other value that line still fails after 6 seconds timeout.
This line works fine: $(".item").shouldHave(text("a"))

This comment has been minimized.

@yashaka

yashaka Oct 27, 2017

Contributor

It was not about waiting 6 seconds. It's about waiting at most 6 seconds and succeed to find elements if they appear or change. Your implementation is not dynamic, it will not succeed. It will remember the exact number of elements at the very beginning - and will not update them after they change during the period of 6 seconds or whatever is set in the Configuration.collectionTimeout

Nevertheless - write a test and check it:)

Just a few tests is not enough for good test coverage of a new feature ;)

You checked only the "subListing" in context of counting size.
But selenide is much more - it is also:

  • dynamic elements that are updated when needed
  • waiting elements
  • informative error messages

I believe we should ensure that all those features are supported by the implementation of new first/last methods, and so we should have tests for them

@yashaka

yashaka Oct 27, 2017

Contributor

It was not about waiting 6 seconds. It's about waiting at most 6 seconds and succeed to find elements if they appear or change. Your implementation is not dynamic, it will not succeed. It will remember the exact number of elements at the very beginning - and will not update them after they change during the period of 6 seconds or whatever is set in the Configuration.collectionTimeout

Nevertheless - write a test and check it:)

Just a few tests is not enough for good test coverage of a new feature ;)

You checked only the "subListing" in context of counting size.
But selenide is much more - it is also:

  • dynamic elements that are updated when needed
  • waiting elements
  • informative error messages

I believe we should ensure that all those features are supported by the implementation of new first/last methods, and so we should have tests for them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment