step_func should be removed in promise_test in dom/animation/test/chrome/.

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: hiro, Assigned: r_kato)

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

From the testharness.js document:

 Note that in the promise chain constructed in test_function assertions don't need to wrapped in step or step_func calls.
r_kato, I'd want you to take this too.

Please remove all t.step_func in promise_test in dom/animation/test.
Thanks!
Flags: needinfo?(ryo_kato)
(Assignee)

Comment 2

2 years ago
Thank you for offering! I will take this too ;)
Flags: needinfo?(ryo_kato)
Assignee: nobody → ryo_kato
This may be covered in bug 1260084.
(Assignee)

Comment 4

2 years ago
I found this bug was related to only animation/test/chrome/test_running_on_compositor.html, which seems not to be covered in bug 1260084. So I will do `hg push review` with r? for the time being.
(Assignee)

Comment 5

2 years ago
Created attachment 8738129 [details]
MozReview Request: Bug 1255682 - Remove unnecessary t.step_func() from a chrome test r=hiro

t.step_func() is the method for event-based callbacks.
In addition, we must call t.done() finally when we use t.step_func().

This test file contains two event-based callbacks:
* L.182 window.requestAnimationFrame()
* L.198 new MutationObserver()

However, given callbacks finally call resolve(), which is suitable for
the spec of promise_test(). So we can remove t.step_func() too.

(There are no t.step_func() we should remove in web-platform tests.)

Review commit: https://reviewboard.mozilla.org/r/44343/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44343/
Depends on: 1260084
I don't think this bug depends on bug 1260084.
No longer depends on: 1260084
Summary: step_func should remove in promise_test → step_func should remove in promise_test in dom/animation/test/chrome/.
Thanks Ryo Kato for doing this!

(In reply to Ryo Kato [:r_kato] from comment #5)
> Created attachment 8738129 [details]
> MozReview Request: Bug 1255682 - Remove unnecessary t.step_func() from a
> chrome test r?
> 
> t.step_func() is the method for event-based callbacks.
> In addition, we must call t.done() finally when we use t.step_func().
> 
> This test file contains two event-based callbacks:
> * L.182 window.requestAnimationFrame()
> * L.198 new MutationObserver()
> 
> However, given callbacks finally call resolve(), which is suitable for
> the spec of promise_test(). So we can remove t.step_func() too.

My comment#1 was wrong.  We can not remove all step_func there.  As you notice there are some assertions in callbacks.
We should pass those assertions to temporary variables with step_func before returning a Promise from promise_test and use the temporary variables in the callbacks.

var checkRunninOnCompositor = t.step_func(function() {
  assert_equals(animation.isRunningOnCompositor, omtaEnabled, ...);
});

return animation.ready.then(function() {
  return new Promise(function(resolve) {
    window.requestAnimationFrame(function() {
      checkRunningOnCompositor();
      resolve();
    });
  });
});

There might be simpler way to use assertions in callbacks in promise_test, but this is the only way I can think of for now.

You can easily confirm that removal of step_func does not break anything if you add an extra assertion which always fails something like assert_true(false).

Thanks!
Summary: step_func should remove in promise_test in dom/animation/test/chrome/. → step_func should be removed in promise_test in dom/animation/test/chrome/.
(Assignee)

Comment 8

2 years ago
Comment on attachment 8738129 [details]
MozReview Request: Bug 1255682 - Remove unnecessary t.step_func() from a chrome test r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44343/diff/1-2/
Attachment #8738129 - Attachment description: MozReview Request: Bug 1255682 - Remove unnecessary t.step_func() from a chrome test r? → MozReview Request: Bug 1255682 - Remove unnecessary t.step_func() from a chrome test r?hiro
Attachment #8738129 - Flags: review?(hiikezoe)
Comment on attachment 8738129 [details]
MozReview Request: Bug 1255682 - Remove unnecessary t.step_func() from a chrome test r=hiro

https://reviewboard.mozilla.org/r/44343/#review41851

The commit message is quite descriptive!  Realy great!

Just one nit.

::: dom/animation/test/chrome/test_running_on_compositor.html:210
(Diff revision 2)
> +
> +        t.step(function() {
> -        assert_true(!!changedAnimation, 'The animation should be recorded '
> +          assert_true(!!changedAnimation, 'The animation should be recorded '
> -          + 'as one of the changedAnimations');
> +            + 'as one of the changedAnimations');
> +        });
> +
> +        t.step(function() {
> -        assert_equals(animation.isRunningOnCompositor, omtaEnabled,
> +          assert_equals(animation.isRunningOnCompositor, omtaEnabled,
> -          'Animation reports that it is running on the compositor'
> +            'Animation reports that it is running on the compositor'
> -           + ' in MutationObserver callback');
> +             + ' in MutationObserver callback');
> +        });

I think single t.step will not be a problem here.
Attachment #8738129 - Flags: review?(hiikezoe) → review+
(Assignee)

Comment 10

2 years ago
https://reviewboard.mozilla.org/r/44343/#review41851

> I think single t.step will not be a problem here.

Thank you for reviewing! I will merge those t.step, then will submit the patch as `r=hiro`.
(Assignee)

Comment 11

2 years ago
Comment on attachment 8738129 [details]
MozReview Request: Bug 1255682 - Remove unnecessary t.step_func() from a chrome test r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44343/diff/2-3/
Attachment #8738129 - Attachment description: MozReview Request: Bug 1255682 - Remove unnecessary t.step_func() from a chrome test r?hiro → MozReview Request: Bug 1255682 - Remove unnecessary t.step_func() from a chrome test r=hiro

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/afdcc508d764
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.