Support multiple elements from selector in assertSee and variants (Dont and/or In) #1151
Conversation
Updated assertions and associated tests to handle multiple elements for given selectors. Replaced `findOrFail` with `all` and modified logic to ensure compatibility with multiple element results. Updated test coverage to reflect these changes.
Updated assertions and associated tests to handle multiple elements for given selectors. Replaced `findOrFail` with `all` and modified logic to ensure compatibility with multiple element results. Updated test coverage to reflect these changes.
|
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
|
Mentioned in #1150 (assertSee() / assertSeeIn() doesn't support selectors to multiple elements) |
|
|
||
| $driver = m::mock(WebDriver::class); | ||
| // $driver->shouldReceive('wait')->with(5, 100)->andReturnUsing(function ($seconds, $interval) use ($driver) { |
There was a problem hiding this comment.
I found wait() was sometimes caled with 5 and not 2, and had to use this commented out version. Could not understand why.
Just realized it's probably because of test_default_wait_time() test higher up, setting static value that persists within the class (Browser::$waitSeconds = 2;), but sometimes not executed - when filtering only this test for example.
It should probably be reset, either in that test (try ... finally{Browser::$waitSeconds = 5;}) or in the class tearDown() method
| @@ -176,12 +176,18 @@ public function assertDontSee($text, $ignoreCase = false) | |||
| public function assertSeeIn($selector, $text, $ignoreCase = false) | |||
| { | |||
| $fullSelector = $this->resolver->format($selector); | |||
| $elements = $this->elements($selector) ?? []; | |||
| // PHPUnit::assertNotEmpty($elements); | |||
There was a problem hiding this comment.
AssertNotEmpty is not strictly required because the assert below will fail, but the "exception" or reason may be different and this may not be desired
| @@ -1346,7 +1346,33 @@ public function test_assert_dont_see() | |||
| } | |||
| } | |||
|
|
|||
| public function test_assert_see_in() | |||
| // public function test_assert_see_in() | |||
There was a problem hiding this comment.
Duplicated, commented out, and renamed below when I thought more than one test would be needed. This should go away and keep the original test name, I guess.
| @@ -53,6 +55,55 @@ public function test_when_available() | |||
| } | |||
| } | |||
|
|
|||
| public function test_when_available_missing() | |||
There was a problem hiding this comment.
These 2 tests reproduce the 2 use cases in test_when_available(), when the item is found and when it's not found.
Although these new tests have some duplication in the mock setup, they are easier to understand, which I think is better, considering the inherent complexity of testing a method that uses a callback
|
Thanks for your pull request to Laravel! Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include. If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions! |
The current implementation of assertSee() / assertSeeIn() / assertDontSee / assertDontSeeIn() support selectors to multiple elements but assume only one element is selected while there are valid/legitimate selector that returns multiple elements.
For example, the content of the first column in a dataTable, ignoring content in other columns: #crudTable > tbody > tr > td:nth-child(1) > span.
I find only the first element is considered and assert on it if it exists.
The asserts do not provide feedback when multiple elements are returned and will be right or wrong depending if the target text is present in the first or in other elements.
After making a macro for additional asserts methods, I found I needed the other (In and/or Dont) variants and the existing code could be extended to manage these cases relatively easily.
The changes transparently support multiple elements
Questions/choices:
Current unit tests use mocks with implementation knowledge and needed changes.
Failed assertion text can be updated to reflect possible multiples, but tests have hard-coded expectations and existing Dusk user tests may also "expect" them as-is.
I wonder about requirement/behavior to fail if no element selected vs searched text being found/absent. It seems it is partially just handling exception from the driver. Maybe I need clarification on that.