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

Do not randomly click the first matching element #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeanmonod
Copy link

Here is my case. A simple web page with a menu and a form:

<nav>
     <a href="/dolphins">Save the dolphins</a>
</nav>

...

<form action="/profile/update">
    <input type="text" name="email">
    <input type="submit" value="Save">
</form>

And here is my test case

    $I->amOnPage('/profile');
    $I->fillField('email', '[email protected]');
    $I->click('Save');
    $I->seeAlertMessage('success', 'Email updated');

In this case, I have a test fail. And it's difficult to understand what was the issue.

So in this PR, I suggest stopping clicking the first matching element on the page, but providing to the developer a appropriate error message about the click that was not precise enough.

Wdyt?

@jeanmonod
Copy link
Author

My example above is quite naive. But when you have two actions on a page, both of which redirect to the same destination, and you don't understand why the action you are testing didn't work. Believe me, it's enough to drive you crazy...

@Naktibalda
Copy link
Member

This is certainly a breaking change and we would have to raise a major version.

Codeception has always worked this way and there are 27 other places in 17 other methods of InnerBrowser that take the first matching element.

@DavertMik
Copy link
Member

@jeanmonod please specify context as the second paramater for this case:

$I->click('Save', 'form');

I agree this behavior could be improved. Not to throw exception but to check for exact matches before clicking "Save dolphins:
And yeah this might be BC break

@jeanmonod
Copy link
Author

there are 27 other places in 17 other methods

Ok, I was not aware of this 😅

Maybe throwing an exception is a bit too much then. My goal was not to break existing tests suites, but to avoid long debugging session while writing new test cases. I don't know conception enough, but possibly you have a mechanism to transmit warning to the developers? Is it the case?

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.

3 participants