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

Fix some shadow root tests #29924

Merged
merged 7 commits into from
Aug 13, 2021

Conversation

sarvaje
Copy link
Contributor

@sarvaje sarvaje commented Aug 5, 2021

This PR fix some remaining issues in the tests for the shadow root commands:

  • Get Element Shadow Root
  • Find Element From Shadow Root
  • Find Elements From Shadow Root

@sarvaje
Copy link
Contributor Author

sarvaje commented Aug 6, 2021

Hi, this is my first contribution and I can find why the tasks are failing. Can you help me, please?

@mathiasbynens
Copy link
Contributor

(For context, this patch is blocking https://chromium-review.googlesource.com/c/chromium/src/+/3033190.)

@foolip
Copy link
Member

foolip commented Aug 10, 2021

@sarvaje what you've run into is #7660. Due to the tests that have been modified, probably too many other tests are considered affected and run 10 times. But there's not enough time to run the 10 times, so both of the task logs end with "Task timeout after 7200 seconds. Force killing container.":
https://community-tc.services.mozilla.com/tasks/McbMml-mQByDrxWWwHiE8w/runs/0/logs/public/logs/live.log
https://community-tc.services.mozilla.com/tasks/L5eda_kSTOKaDau2omgoCQ/runs/0/logs/public/logs/live.log

When this happens, the only recourse is to ping @web-platform-tests/admins asking someone to admin merge, basically ignoring the failed CI checks.

If the tests are in order and you have a review but can't merge it, feel free to ping me or @past for merging.

@sarvaje
Copy link
Contributor Author

sarvaje commented Aug 10, 2021

@foolip Thanks for your answer, before pinging @web-platform-tests/admins for the merge, should I wait for someone to approve my changes, right?

@foolip
Copy link
Member

foolip commented Aug 10, 2021

Right, you'll need someone to review the actual changes here. In all honestly that probably won't happen by just waiting, so you might want to look at who has recently written or reviewed tests close to these and ping them directly until you find a reviewer.

@sarvaje
Copy link
Contributor Author

sarvaje commented Aug 10, 2021

Ok, thanks!!

@sarvaje
Copy link
Contributor Author

sarvaje commented Aug 10, 2021

@whimboo I see you reviewed the PR that added the Shadow root tests. Can you take a look to my PR too? Thanks!

Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your contribution! It's great to see these obvious mistakes to be fixed soon. Nevertheless I added some inline comments that have to be looked at.

tools/webdriver/webdriver/client.py Outdated Show resolved Hide resolved
webdriver/tests/find_element_from_shadow_root/find.py Outdated Show resolved Hide resolved
webdriver/tests/find_elements_from_shadow_root/find.py Outdated Show resolved Hide resolved
webdriver/tests/get_element_shadow_root/get.py Outdated Show resolved Hide resolved
webdriver/tests/support/asserts.py Outdated Show resolved Hide resolved
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me now.

@sarvaje
Copy link
Contributor Author

sarvaje commented Aug 12, 2021

Thanks @whimboo.
The checks failing are because of a timeout. @foolip mention that I can ping @web-platform-tests/admins to bypass the checks and get the PR merged. How can I ping them, github doesn't recognize @web-platform-tests/admins as a know group or anything hehehe.

Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually tests fail because of the changes to the Element class:

url = '/session/84b28476-db5d-488a-8bb4-da75dde1ec5f/element/<Session 84b28476-db5d-488a-8bb4-da75dde1ec5f>/value'

As such please revert these, to not cause massive extra work.

@sarvaje
Copy link
Contributor Author

sarvaje commented Aug 12, 2021

As such please revert these, to not cause massive extra work.

done

Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, all tests passing now and stability ones timeout as expected. Thanks!

@jgraham jgraham merged commit db06586 into web-platform-tests:master Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants