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

#617 fix: Changed PageFactory to SelenidePageFactory in ElementsContainer's fields initialization #618

Merged
merged 2 commits into from Oct 22, 2017

Conversation

Projects
None yet
5 participants
@CaBocuk

CaBocuk commented Oct 8, 2017

Proposed changes

#617 fix.
Changed PageFactory to SelenidePageFactory in ElementsContainer's fields initialization what made Selenide not to re-init the fields that are already initialized.

BUT I realized that for now there is almost no sense in using not @findby in ElementsContainer because:

  • If you do SelenideElement el = $(...); -> Selenide uses WebDriver as search context but not your container. So you have to specify locator which should include finding the parent element (container)
  • SelenideElement el = getSelf().$(...) throws NPE just because page(SomeContainer.class) is called after the Container being instantiated itself. So self is null when getSelf().$(...) is called.
    So probably that would be good to review the concept of ElementsContainer because there are too much nuances in it.

Checklist

  • Checks passed locally
  • Test added
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 8, 2017

Coverage Status

Coverage increased (+0.1%) to 64.432% when pulling 24191f6 on CaBocuk:#617-Fix-ElementsContainer-with-initialized-elements into 01774b9 on codeborne:master.

coveralls commented Oct 8, 2017

Coverage Status

Coverage increased (+0.1%) to 64.432% when pulling 24191f6 on CaBocuk:#617-Fix-ElementsContainer-with-initialized-elements into 01774b9 on codeborne:master.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 8, 2017

Codecov Report

Merging #618 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #618      +/-   ##
============================================
+ Coverage     60.51%   60.65%   +0.14%     
- Complexity      766      767       +1     
============================================
  Files           148      148              
  Lines          2730     2730              
  Branches        267      267              
============================================
+ Hits           1652     1656       +4     
+ Misses          974      971       -3     
+ Partials        104      103       -1
Impacted Files Coverage Δ Complexity Δ
...odeborne/selenide/impl/SelenideFieldDecorator.java 83.33% <100%> (ø) 15 <0> (ø) ⬇️
...e/selenide/impl/WebDriverThreadLocalContainer.java 82.6% <0%> (+2.89%) 31% <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 01774b9...24191f6. Read the comment docs.

codecov-io commented Oct 8, 2017

Codecov Report

Merging #618 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #618      +/-   ##
============================================
+ Coverage     60.51%   60.65%   +0.14%     
- Complexity      766      767       +1     
============================================
  Files           148      148              
  Lines          2730     2730              
  Branches        267      267              
============================================
+ Hits           1652     1656       +4     
+ Misses          974      971       -3     
+ Partials        104      103       -1
Impacted Files Coverage Δ Complexity Δ
...odeborne/selenide/impl/SelenideFieldDecorator.java 83.33% <100%> (ø) 15 <0> (ø) ⬇️
...e/selenide/impl/WebDriverThreadLocalContainer.java 82.6% <0%> (+2.89%) 31% <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 01774b9...24191f6. Read the comment docs.

@BorisOsipov

This comment has been minimized.

Show comment
Hide comment
@BorisOsipov

BorisOsipov Oct 11, 2017

Member

Hi @CaBocuk

Hmm I don't understand what "So self is null when getSelf().$(...) is called." means.
Can you describe?

Member

BorisOsipov commented Oct 11, 2017

Hi @CaBocuk

Hmm I don't understand what "So self is null when getSelf().$(...) is called." means.
Can you describe?

@BorisOsipov

This comment has been minimized.

Show comment
Hide comment
@BorisOsipov

BorisOsipov Oct 14, 2017

Member

Hmm I don't understand what "So self is null when getSelf().$(...) is called." means.

Never mind. I understood what you mean :)

Member

BorisOsipov commented Oct 14, 2017

Hmm I don't understand what "So self is null when getSelf().$(...) is called." means.

Never mind. I understood what you mean :)

@CaBocuk

This comment has been minimized.

Show comment
Hide comment
@CaBocuk

CaBocuk Oct 14, 2017

Never mind. I understood what you mean :)

Great! :)
Maybe some comments about the change?

CaBocuk commented Oct 14, 2017

Never mind. I understood what you mean :)

Great! :)
Maybe some comments about the change?

@BorisOsipov BorisOsipov added the ready label Oct 15, 2017

@BorisOsipov

This comment has been minimized.

Show comment
Hide comment
@BorisOsipov
Member

BorisOsipov commented Oct 15, 2017

LGTM

@asolntsev asolntsev self-assigned this Oct 22, 2017

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

@asolntsev asolntsev merged commit 5fb6350 into selenide:master Oct 22, 2017

4 checks passed

codecov/patch 100% of diff hit (target 60.51%)
Details
codecov/project 60.65% (+0.14%) compared to 01774b9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 64.432%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment