Harden window closing code upon session reset #27
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
============================================
- Coverage 83.65% 82.24% -1.42%
+ Complexity 192 191 -1
============================================
Files 1 1
Lines 471 473 +2
============================================
- Hits 394 389 -5
- Misses 77 84 +7 ☔ View full report in Codecov by Sentry. |
aik099
force-pushed
the
session-reset-with-initial-window-rename-fix
branch
from
870aa71 to
7d71e14
Compare
…andle conversion code)
tests/Custom/SessionResetTest.php
Outdated
| /** | ||
| * @dataProvider initialWindowNameDataProvider | ||
| */ | ||
| public function testSessionResetClosesWindows(?string $initialWindowName): void |
There was a problem hiding this comment.
@stof Would it make sense to put this test in driver-testsuite?
There was a problem hiding this comment.
I've added this test into the driver's test suite instead of the Mink driver test suite because the window closing behavior upon session reset is non-standard or advertised anywhere.
There was a problem hiding this comment.
I would suggest adding that in the driver testsuite as I think it makes sense for any driver supporting to open new windows to close them on reset.
We can update the phpdoc of the DriverInterface to suggest drivers to close extra windows.
There was a problem hiding this comment.
I would suggest adding that in the driver testsuite as I think it makes sense for any driver supporting to open new windows to close them on reset.
Done in the minkphp/driver-testsuite#96 .
We can update the phpdoc of the DriverInterface to suggest drivers to close extra windows.
@stof , please review my other PRs (7 by today). It's been over a month now, and I don't know if you've looked at them and they're OK to merge or I need to change something.
Also what can I do to speed up the review process?
Once that is arranged I can try creating a DriverInterface-related PR.
aik099
force-pushed
the
session-reset-with-initial-window-rename-fix
branch
from
8198a7b to
da234b2
Compare
|
Thank you, @stof . |
Problems solved:
Session::resetcloses main window, when window was renamed #25)NoSuchWindowexception being thrown upon session reset, when initial window name was changed through the JavaScript after opening (Closes TheSession::resetthrows an exception when main window has name #26)Test failure is unrelated.