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

Added because method to CollectionCondition(#440) #749

Merged
merged 3 commits into from Jun 27, 2018

Conversation

Projects
None yet
4 participants
@sidelnikovmike
Contributor

sidelnikovmike commented May 28, 2018

See #440.
WIP

Checklist

  • Checkstyle and unit tests pass locally with my changes by running фд command
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
@codecov-io

This comment has been minimized.

codecov-io commented May 28, 2018

Codecov Report

Merging #749 into master will decrease coverage by 0.32%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #749      +/-   ##
============================================
- Coverage     63.66%   63.34%   -0.33%     
+ Complexity      879      878       -1     
============================================
  Files           154      154              
  Lines          3030     3039       +9     
  Branches        298      298              
============================================
- Hits           1929     1925       -4     
- Misses         1003     1015      +12     
- Partials         98       99       +1
Impacted Files Coverage Δ Complexity Δ
...va/com/codeborne/selenide/CollectionCondition.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...e/selenide/impl/WebDriverThreadLocalContainer.java 80.74% <0%> (-2.97%) 30% <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 ab64b3b...4657c66. Read the comment docs.

@coveralls

This comment has been minimized.

coveralls commented May 28, 2018

Coverage Status

Coverage decreased (-0.3%) to 66.601% when pulling 4657c66 on sidelnikovmike:pr_440 into ab64b3b on codeborne:master.

@sidelnikovmike

This comment has been minimized.

Contributor

sidelnikovmike commented May 28, 2018

I'll try to add more tests for this method

@sidelnikovmike

This comment has been minimized.

Contributor

sidelnikovmike commented May 28, 2018

Some questions:

  1. I've add one more test, but code coverage decrease becomes more.
  2. @asolntsev I've found, that custom because message not added to fail :( As I understand, this is because we don't know in Errors about this custom message. Previous author adds new ExplainedUIAssertionError class for this. But it looks like not so pretty solution.
    But in other case - we need to pass message or CollectionCondition(as it is for simple Condition class) to implementations of UIAssertionError.
    Do you have any ideas, how to make this code pretty? :)
@sidelnikovmike

This comment has been minimized.

Contributor

sidelnikovmike commented Jun 7, 2018

@asolntsev any ideas?

@asolntsev

This comment has been minimized.

Contributor

asolntsev commented Jun 7, 2018

@sidelnikovmike I need time to think about it...

@sidelnikovmike

This comment has been minimized.

Contributor

sidelnikovmike commented Jun 7, 2018

@asolntsev you are welcome 👍

@asolntsev

This comment has been minimized.

Contributor

asolntsev commented Jun 26, 2018

@sidelnikovmike Sorry for the delay. It seems that the simplest solution would be to move because method inside CollectionCondition. See my commit: 40effdb

What do you think?

@sidelnikovmike

This comment has been minimized.

Contributor

sidelnikovmike commented Jun 27, 2018

@asolntsev Yes, it seems the better solution! How we could merge it? I see that you do it in branch in this repo. Should I recreate PR with this code? or we'll close this PR and you open new one?

@asolntsev asolntsev added this to the 4.12.2 milestone Jun 27, 2018

@asolntsev asolntsev self-assigned this Jun 27, 2018

@asolntsev asolntsev added the feature label Jun 27, 2018

@asolntsev asolntsev merged commit 6d18f20 into selenide:master Jun 27, 2018

1 of 4 checks passed

codecov/patch 0% of diff hit (target 63.66%)
Details
codecov/project 63.34% (-0.33%) compared to ab64b3b
Details
coverage/coveralls Coverage decreased (-0.3%) to 66.601%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

asolntsev added a commit that referenced this pull request Jun 27, 2018

asolntsev added a commit that referenced this pull request Jun 27, 2018

@asolntsev

This comment has been minimized.

Contributor

asolntsev commented Jun 27, 2018

@sidelnikovmike No problems. I have merged your PR and then my commit. Will be released in Selenide 4.12.2

@sidelnikovmike

This comment has been minimized.

Contributor

sidelnikovmike commented Jun 27, 2018

@asolntsev sounds great! we'll wait for this! Thanks!

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