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 a bug in test_driver.bless() #49719

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Fix a bug in test_driver.bless() #49719

merged 1 commit into from
Dec 17, 2024

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Dec 16, 2024

This code was previously doing this:

let wait_click = new Promise(resolve => {
button.addEventListener("click", resolve));
};
return test_driver.click(button)
.then(wait_click)
.then(...

but the argument to .then(wait_click) isn't a function, it's the
promise to return. Therefore .then() creates an already-resolved
promise containing wait_click as its resolved value. Which the next
.then() block ignores. So this wasn't actually waiting for the click
to occur.

This triggered a number of test bugs (caused by erroneous assumptions
accidentally baked into the tests. I fixed a few, and filed a few
bugs for the rest (after failing to figure out how to fix them).

Note that the WPT version of testdriver.js is rolled into Chromium,
so that change is being made here:
#49691

Bug: 384009734,384050894
Change-Id: Ibdb8a97d23998ad89c5a48c23a7e780dc605283b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6094526
Reviewed-by: Jonathan Lee <jonathanjlee@google.com>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Commit-Queue: Jonathan Lee <jonathanjlee@google.com>
Cr-Commit-Position: refs/heads/main@{#1397010}

This code was previously doing this:

  let wait_click = new Promise(resolve => {
    button.addEventListener("click", resolve));
  };
  return test_driver.click(button)
    .then(wait_click)
    .then(...

but the argument to `.then(wait_click)` isn't a function, it's the
promise to return. Therefore .then() creates an already-resolved
promise containing `wait_click` as its resolved value. Which the next
`.then()` block ignores. So this wasn't actually waiting for the click
to occur.

This triggered a number of test bugs (caused by erroneous assumptions
accidentally baked into the tests. I fixed a few, and filed a few
bugs for the rest (after failing to figure out how to fix them).

Note that the WPT version of testdriver.js is rolled into Chromium,
so that change is being made here:
  #49691

Bug: 384009734,384050894
Change-Id: Ibdb8a97d23998ad89c5a48c23a7e780dc605283b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6094526
Reviewed-by: Jonathan Lee <jonathanjlee@google.com>
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Commit-Queue: Jonathan Lee <jonathanjlee@google.com>
Cr-Commit-Position: refs/heads/main@{#1397010}
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

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

The review process for this patch is being conducted in the Chromium project.

@past
Copy link
Member

past commented Dec 17, 2024

@mfreed7 are the Safari test failures expected?

@mfreed7
Copy link
Contributor

mfreed7 commented Dec 17, 2024

@mfreed7 are the Safari test failures expected?

Interesting. So two things changed in this PR, one is the change to test_driver and the other was a cleanup of the one failing Safari test. The failure there doesn't appear related to the test_driver change. It would seem that perhaps there's a bug in Safari with user activation and showPicker()? The only real change for all of the newly-failing tests is that the test now (correctly) adds the element to the document before calling showPicker(). I'm up for comments on what I changed in the test, but it looks "ok" to me.

Copy link
Member

@past past left a comment

Choose a reason for hiding this comment

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

Looks good to me too.

@past
Copy link
Member

past commented Dec 17, 2024

The new failures seem like legitimate bugs, so I will merge this.

@past past merged commit ce746b4 into master Dec 17, 2024
25 checks passed
@past past deleted the chromium-export-cl-6094526 branch December 17, 2024 21:30
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.

4 participants