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

Possible fix for a 'selectOption' issue #408

Merged
merged 4 commits into from Feb 25, 2017

Conversation

Projects
None yet
3 participants
@robrkerr
Contributor

robrkerr commented Feb 6, 2017

I had what appears to be the same issue as in #295, where the select element was correctly found and the correct option selected but then this error was thrown:

Failed to execute 'iterateNext' on 'XPathResult': The document has mutated since the result was returned.

So I changed the implementation slightly so that it no longer cycles through different options - it just selects the first one that matches.

Note: this won't work for selecting multiple options - it will only work for a dropdown select. But given this is the more common one I thought it was much more crucial it work for this. Obviously a better solution would be for it to work on both but I don't know enough about what's going on to work that out.

@DavertMik DavertMik added the nightmare label Feb 6, 2017

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Feb 6, 2017

Member

Looks like it broke current selectOption tests. To reproduce them and fix run:

Start a server with demo app (yes, you need PHP for that):

php -S 127.0.0.1:8000 -t test/data/app
mocha tests/helpers/Nightmare_test.js --grep "selectOption"

Member

DavertMik commented Feb 6, 2017

Looks like it broke current selectOption tests. To reproduce them and fix run:

Start a server with demo app (yes, you need PHP for that):

php -S 127.0.0.1:8000 -t test/data/app
mocha tests/helpers/Nightmare_test.js --grep "selectOption"

@sqmgh

This comment has been minimized.

Show comment
Hide comment
@sqmgh

sqmgh Feb 7, 2017

The tests are likely failing because he changed/broke current behavior.

The error that is being worked around is pretty 'standard' for iterators. Just pushing the results into an array, and then looping through that instead would fix things.

Hacked together like:

      var current = null;
      var items = [];
      while (current = found.iterateNext()) {
        items.push(current);
      }
      for (var i = 0; i < items.length; items++) {
        current = items[i];

Of course, I don't know the exact behavior that is expected by the test. This method of fixing things doesn't really mimic the exact behavior of a user in all cases.
There could be a scenario where clicking one option could remove another from the select. Removing the user's ability to actually click the second item. If that's important, the fix would need to be slightly more complicated.

Forgive me for not posting a proper PR for this. I stumbled here searching for the error message while previewing CodeceptJS to see if it was suitable for my project needs. I have neither the time or inclination to get php and a testing environment set up to do things properly. However, the code change I have posted did unblock me from running my specific sample test.

sqmgh commented Feb 7, 2017

The tests are likely failing because he changed/broke current behavior.

The error that is being worked around is pretty 'standard' for iterators. Just pushing the results into an array, and then looping through that instead would fix things.

Hacked together like:

      var current = null;
      var items = [];
      while (current = found.iterateNext()) {
        items.push(current);
      }
      for (var i = 0; i < items.length; items++) {
        current = items[i];

Of course, I don't know the exact behavior that is expected by the test. This method of fixing things doesn't really mimic the exact behavior of a user in all cases.
There could be a scenario where clicking one option could remove another from the select. Removing the user's ability to actually click the second item. If that's important, the fix would need to be slightly more complicated.

Forgive me for not posting a proper PR for this. I stumbled here searching for the error message while previewing CodeceptJS to see if it was suitable for my project needs. I have neither the time or inclination to get php and a testing environment set up to do things properly. However, the code change I have posted did unblock me from running my specific sample test.

@robrkerr

This comment has been minimized.

Show comment
Hide comment
@robrkerr

robrkerr Feb 19, 2017

Contributor

Ok - I worked out that the error was occurring due to changes to the DOM due to an 'onchange' event listener. I've recreated this in a test and following the suggested code changes from @sqmgh (thanks!) both this new test and the existing ones are passing.

Contributor

robrkerr commented Feb 19, 2017

Ok - I worked out that the error was occurring due to changes to the DOM due to an 'onchange' event listener. I've recreated this in a test and following the suggested code changes from @sqmgh (thanks!) both this new test and the existing ones are passing.

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Feb 25, 2017

Member

Thank you for the deep investigation and a good fix 👍

Member

DavertMik commented Feb 25, 2017

Thank you for the deep investigation and a good fix 👍

@DavertMik DavertMik merged commit 2cee168 into Codeception:master Feb 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment