-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[webdriver] Add character typing tests #32679
base: master
Are you sure you want to change the base?
[webdriver] Add character typing tests #32679
Conversation
[u'\ue028', "."], | ||
[u'\ue029', "/"], | ||
[u'\ue01a', "0"], | ||
[u'\ue023', "9"] |
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.
Question: What about all the other numbers? Also it misses \ue026
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.
I didn't add all the values because wpt times out the test file if there are a lot of parametrised values. I can raise an issue but I can't solve that issue in this test PR.
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.
Ah that one. Now I remember. Do you actually have a link to such a job in the wpt CI? I would be interested in. But yes it's fine to file an issue and add a TODO comment here with a link to that issue.
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.
Do you have such a link to a broken job in CI?
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.
I didn't put it through CI. It was timing out locally. I assumed it would do the same locally as on the CI
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.
As discussed yesterday the underlying problem with Chrome is gone and personally I also do not see any timeout when running way more parameterized tests with Chrome. So I would suggest that we do not land the patch in a rush but get coverage for all the normalized keys.
I'm happy to update the PR in case you don't have the time for.
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.
I have added a few more. I think it's important to get this landed than make sure we have all the cases covered and we can add cases later when someone has time
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.
Ok, I can see that Chrome and also Firefox is running into a timeout. When I have a look at the trace logs at least for Firefox locally I can see that there is not that much of an improvement that we can do beside the following:
- Use a
# META: timeout=long
line at the very top which in this case would be fine to do. - For tests where we can combine characters lets do it so that we can safe the time as needed for setup/teardown and especially loading the test page.
- Think about actually loading real data URIs instead of inline docs which will still be routed through wptserve.
Especially 2) will help a lot here given that we spend 50% of the test's time in setup/teardown.
[u'\ue01a', "0"], | ||
[u'\ue023', "9"] | ||
]) | ||
def test_normalised_key_value(session, inline, value): |
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.
This test doesn't cover all the normalized keys. Maybe add printable
to the test name?
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.
I could but I don't see value in that. It's only printable because I limited the characters due to timeouts happening in the tests if there are more parametrised values.
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.
Non-printable keys won't be testable this way and we might have to use an event listener instead to ensure that the correct key got pressed. As such you cannot just extend the parameterized list.
Co-authored-by: Henrik Skupin <[email protected]>
Co-authored-by: Henrik Skupin <[email protected]>
[u'\ue028', "."], | ||
[u'\ue029', "/"], | ||
[u'\ue01a', "0"], | ||
[u'\ue023', "9"] |
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.
Do you have such a link to a broken job in CI?
review fix Co-authored-by: Henrik Skupin <[email protected]>
@whimboo I've update things based on your comments |
@AutomatedTester mind to also check my other comments where I'm awaiting a reply for? #32679 (comment) thanks |
@pytest.mark.parametrize("value",[ | ||
[u'\ue018', ";"], |
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.
What I didn't notice before I actually run these tests locally was that for parameterization you have to use tuples. At least for me pytest complains about that. Also it avoids to have to call the indices.
@pytest.mark.parametrize("value",[ | |
[u'\ue018', ";"], | |
@pytest.mark.parametrize("value, expected",[ | |
(u'\ue018', ";"), |
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.
updated
[u'\ue028', "."], | ||
[u'\ue029', "/"], | ||
[u'\ue01a', "0"], | ||
[u'\ue023', "9"] |
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.
As discussed yesterday the underlying problem with Chrome is gone and personally I also do not see any timeout when running way more parameterized tests with Chrome. So I would suggest that we do not land the patch in a rush but get coverage for all the normalized keys.
I'm happy to update the PR in case you don't have the time for.
# TODO This is not extensive list due to timeouts from wpt if there are a lot of test | ||
# See https://github.com/web-platform-tests/wpt/issues/32899 |
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.
This issue is no longer needed I assume and we can close?
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.
I still hit this issue in Chrome.
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.
I parameterized the bottom test case in the file and that hits it on my local machine with chrome.
@AutomatedTester sorry but it looks like we or I dropped the ball a while ago for this pull request. Could you please check what's left to do so that we can get it landed? There are a couple of open issues, and not sure which are resolved and which are not. Thanks. |
@AutomatedTester it's been a while and I would like to know if you are still interested in these tests. There is a conflict as well. Thanks. |
No description provided.