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

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

r_kato, I'd want you to take this too.

Please remove all t.step_func in promise_test in dom/animation/test.
Thanks!

Comment 2

[Ryo Kato [:r_kato]]

Thank you for offering! I will take this too ;)

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

This may be covered in bug 1260084.

Comment 4

[Ryo Kato [:r_kato]]

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.

Comment 5

[Ryo Kato [:r_kato]]

Attachment: 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/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

I don't think this bug depends on bug 1260084.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

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!

Comment 8

[Ryo Kato [:r_kato]]

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/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 10

[Ryo Kato [:r_kato]]

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`.

Comment 11

[Ryo Kato [:r_kato]]

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/

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afdcc508d764

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/afdcc508d764