Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jeffreyguo
Copy link

Added useful methods to get elements:

  • getAllChildren()
  • getDirectChildren()
  • getFirstChild()
  • getLastChild()
  • getPreviousSiblings()
  • getPreviousSibling()
  • getNextSiblings()
  • getNextSibling()
  • findByName($name)
  • findByTag($tag)

@aik099 aik099 changed the title added more methods to get child and sibling elements: getAllChildren(), getDirectChildren(), getFirstChild(), getLastChild(), findByName($name), findByTag($tag) More methods for NodeElement children manipulations Jul 22, 2014
@aik099
Copy link
Member

aik099 commented Jul 22, 2014

@jeffreyguo please use title and description fields of the issue/PR on GitHub appropriately. Do not write long texts in the title field.

@aik099
Copy link
Member

aik099 commented Jul 22, 2014

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()
Copy link
Member

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.

@aik099
Copy link
Member

aik099 commented Jul 22, 2014

Actually most of the methods doesn't belong to NodeElement, but rather to Traversable class.

@jeffreyguo
Copy link
Author

@aik099 I saw getParent() is in NodeElement.php, so I thought these getXXX methods could be placed here also.

@aik099
Copy link
Member

aik099 commented Jul 23, 2014

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.

@aik099
Copy link
Member

aik099 commented Jul 23, 2014

Ah, I remembered one reason why find* methods are in NodeElement. The architecture (part) of a Mink is as follows:
mink_architecture

For that reason, the prev/next and other majority of methods doesn't belong to TraversableElement since they don't apply to DocumentElement, that will inherit them. That's why they are placed in NodeElement.php.

P.S.
When you make a change in commits, then please write a comment about it, because GitHub doesn't send notifications when just commit changes.

@jeffreyguo
Copy link
Author

New commit [c499b97]: Added replacement %nameMatch% for new method findByName and updated comment's format

@jeffreyguo
Copy link
Author

New commit [0f20bfe]: Corrected the element name for nameselector name test

@jeffreyguo
Copy link
Author

New commit [d832fd6]: Updated expected result from 1 to 11 for name test and passed

@jeffreyguo
Copy link
Author

New commit [ef96fdb]: Updated name selector for partial search

@aik099
Copy link
Member

aik099 commented Jul 25, 2014

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. getChildren method) or similar ever.

@jeffreyguo
Copy link
Author

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.

@aik099
Copy link
Member

aik099 commented Jul 25, 2014

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
Copy link
Member

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)

@stof
Copy link
Member

stof commented Aug 13, 2014

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants