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

Adding a few setTimeouts to the utility function #25161

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thejohnjansen
Copy link
Contributor

@thejohnjansen thejohnjansen commented Aug 20, 2020

The timing in these tests is just a little of when these tests are automated. This is meant to address: #25023

…s just a little of when these tests are automated
@stephenmcgruer
Copy link
Contributor

stephenmcgruer commented Aug 21, 2020

So knowing nothing about how the tests work, if the goal is to wait for the callback from the IntersectionObserver before checking things, would a promise-based test be useful? E.g. something like:

promise_test(async t => {
  let entries;
  let observerPromise = new Promise(resolve => {
    let observer = new IntersectionObserver(function(changes) {
        entries = changes;
        resolve();
    });
  });
  // do some stuff
  await observerPromise;
  // verify entries
}, 'my test');

But perhaps there is some complexity in needing to wait for multiple calls to the changes function or something like that?

Generally speaking, adding timeouts like this is only going to make a test flaky (particularly since they seem quite arbitrary in this diff).

Hopefully @szager-chromium can chime in, however, as someone who knows IntersectionObserver wayyyyyy better than me :)

@thejohnjansen
Copy link
Contributor Author

Thanks for the comment @stephenmcgruer I pushed new changes to make it simpler before I read your comment, don't want you to think I was ignoring you. I'll look forward to hearing from @szager-chromium as well. For these tests I think the complexity is in the requestAnimationFrame, but I don't know exactly how these work either.

@thejohnjansen
Copy link
Contributor Author

thejohnjansen commented Aug 27, 2020

Would it be possible to merge this and discuss the possibility of a re-write to promises with the Working Group? I don't like that wpt.fyi shows inaccurate test results for this. I'm sure there are probably other places as well that I am going to start looking into.

I just remembered about the 2way sync that was enabled some time ago with chromium. If it makes more sense, I could make this change in the chromium repo.

@szager-chromium
Copy link
Contributor

I made a comment on the bug -- tl;dr I am pretty strongly against landing this change.

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