-
Notifications
You must be signed in to change notification settings - Fork 280
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
More methods for NodeElement children manipulations #586
base: master
Are you sure you want to change the base?
Conversation
@jeffreyguo please use |
I recommend you to synchronize GitHub username/e-mail used to login to GitHub and one set in your Git client. Without this you have 2 different avatars in your commits and your GitHub account. |
* | ||
* @return NodeElements|null | ||
*/ | ||
public function getAllChildren() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct indentation. I guess you're using TABs here (in in other places in this PR), which doesn't comply with PSR-2.
Actually most of the methods doesn't belong to |
@aik099 I saw getParent() is in NodeElement.php, so I thought these getXXX methods could be placed here also. |
Besides, what can be the use case for these methods? Considering that 1 element location operations takes time, then whole test suite can be really slow if you will traverse through element tree without actual knowing of what you're looking for. |
New commit [c499b97]: Added replacement |
New commit [0f20bfe]: Corrected the element name for nameselector |
New commit [d832fd6]: Updated expected result from 1 to 11 for |
New commit [ef96fdb]: Updated name selector for partial search |
Now it looks good. What I wonder is what can be the application of these methods in real life? Mink is supposed to help in acceptance/functional testing. I've never seen, that somebody would do that massive element traversal (e.g. |
Good question. For automated testing, object identification is the most important thing. Sometimes there is no id, no name, and no one attribute can be relied on. At this case, relative path is useful, we can get an object by identifing its parent/sibling firstly, that's why getFirstchild()/getLastChild/getPreviousSibling()/getNextSibling() here. With regard to getAllChildren(), it's useful in web Table test, e.g. get all cells in a table row. |
I see. I haven't tested tables myself. Usually I place ID/class all over the place to be able to locate elements easily. |
@@ -56,6 +56,86 @@ public function getParent() | |||
} | |||
|
|||
/** | |||
* Returns All child elements (including descendant) to the current one. | |||
* | |||
* @return static[]|null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findAll
can never return null
.
and it should be NodeElement[]
or self[]
, not static[]
. If you extend the NodeElement class, the elements returned here will not use the child class (findAll()
always returns a list of NodeElement instances)
these new methods should be covered by tests (both unit tests running in this repo testsuite, and functional tests running in the driver testsuite IMO) |
Added useful methods to get elements: