setSelected method should check value after .click() action #520

[jane-ryabchenko]

Added SetSelected.isSelectable and SetSelected.isDeselectable methods.
If user tries to select not selectable element InvalidStateException will be thrown.
If user tries to de-select radio or option element InvalidStateException will be thrown.
Added Unit and Integration tests to cover every case.

[SergeyPirogov]

Well, guys with modern UI frameworks it's all useless!

[coveralls]

Coverage increased (+0.2%) to 64.436% when pulling d441a3a on jane-ryabchenko:fix-selection-command into cd259dd on codeborne:master.

[codecov-io]

Codecov Report

Merging #594 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #594      +/-   ##
============================================
+ Coverage     60.43%   60.48%   +0.05%     
+ Complexity      770      734      -36     
============================================
  Files           148      134      -14     
  Lines          2750     2660      -90     
  Branches        270      273       +3     
============================================
- Hits           1662     1609      -53     
+ Misses          982      945      -37     
  Partials        106      106

Impacted Files	Coverage Δ	Complexity Δ
...a/com/codeborne/selenide/commands/SetSelected.java	100% <100%> (ø)	15 <7> (+11)	⬆️
...codeborne/selenide/webdriver/WebDriverFactory.java	64.7% <0%> (-17.44%)	31% <0%> (+19%)
...ne/selenide/impl/WebElementsCollectionWrapper.java	83.33% <0%> (-16.67%)	2% <0%> (-1%)
...e/selenide/impl/WebDriverThreadLocalContainer.java	76.55% <0%> (-3.89%)	29% <0%> (-1%)
.../java/com/codeborne/selenide/ex/ErrorMessages.java	82.6% <0%> (-3.6%)	13% <0%> (-4%)
...in/java/com/codeborne/selenide/impl/Navigator.java	65.62% <0%> (-3.23%)	21% <0%> (ø)
.../codeborne/selenide/impl/ScreenShotLaboratory.java	53.52% <0%> (-0.97%)	34% <0%> (ø)
src/main/java/com/codeborne/selenide/Selenide.java	0% <0%> (ø)	0% <0%> (ø)	⬇️
...n/java/com/codeborne/selenide/WebDriverRunner.java	0% <0%> (ø)	0% <0%> (ø)	⬇️
...om/codeborne/selenide/impl/WebElementSelector.java	65.51% <0%> (ø)	11% <0%> (ø)	⬇️
... and 17 more

---

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 70ba5a8...f1ff877. Read the comment docs.

[jane-ryabchenko]

@SergeyPirogov did you mean that this pull request will never be reviewed/merged ?

[asolntsev]

@jane-ryabchenko Not exactly :)

But yes, it's true: I am in doubt. I am afraid that this change will make things slower, but will not always solve the problem. I don't claim that your PR is definitely wrong, I am just in doubt.

Anyway, let's start with a simple question:

1. What problem are you trying to solve? What is the real case?
2. Why your change doesn't correspond to the title? Title says "setSelected method should check value AFTER .click() action", but actually it checks something else (type, not value) and BEFORE action.

[jane-ryabchenko]

@asolntsev
I'll start with second question.

Required change described in #520 doesn't makes much sense to me.
What is the point of reckless clicking of the element and checking if it was right element in right state after click?

For example, if I call checkbox.setSelected(true) I expect that checkbox.isSelected() will return true after the call. But actual code in setSelected method does not checks the state of the element to determine if it's already selected and shouldn't be clicked. Definitely I don't want to be in a situation when checkbox is already selected and call checkbox.setSelected(true) sets selection to false.

[coveralls]

Coverage increased (+0.3%) to 64.662% when pulling f1ff877 on jane-ryabchenko:fix-selection-command into d9cf38b on codeborne:master.

[coveralls]

Coverage increased (+0.1%) to 64.474% when pulling f1ff877 on jane-ryabchenko:fix-selection-command into d9cf38b on codeborne:master.

[rosolko]

@jane-ryabchenko, @asolntsev, @BorisOsipov I my point of view this case mix functionality with verification.
I think the real use case will we:

1. Set selected.
2. Make assert that check condition.
3. You decide check or not.
4. No performance issues.
   Are you agree?
   So If yes - we can close this PR and check such cases on your side.

@jane-ryabchenko Just note for you you shouldn't commit .idea folder because it's user specific.

[BorisOsipov]

First, I don't understand what changes are proposed to this PR - diff is so big and full of garbage. I guess it makes sense to create a new PR with corresponding commits.

Second, I agree with @SergeyPirogov "Well, guys with modern UI frameworks it's all useless!" - it is the sad truth. In modern UI frameworks you hardly ever deal with standard html select.

[rosolko]

Close it for now, if really needed submit a new PR cleared from not necessary files.