Closed Bug 1256503 Opened 4 years ago Closed 4 years ago

Longer digits in test codes should be calculated with a const value in place

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: hiro, Unassigned)

Details

Attachments

(3 files)

As we all have been noticed, longer digits, say 10000, are hard to read.  We should replace it by human readable one.

dholbert suggested MS_PER_SEC to me in bug 1254840 comment#11.  We should define MS_PER_SEC in testcommons.js as a const variable and use it!
These patch are based upon patches for bug 1255710.

There are still raw div.animate() in test_animation_performance_warning.html because there needs to be created a couple of animations on the same element.
Comment on attachment 8735335 [details]
MozReview Request: Bug 1256503 - Part 1: Use MS_PER_SEC instead of human-misreadable digits in animation tests. r?dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42741/diff/1-2/
Attachment #8735335 - Attachment description: MozReview Request: Bug 1256503 - Part 1: Use MS_PER_SEC instead of human-misreadable digits. r?birtles → MozReview Request: Bug 1256503 - Part 1: Use MS_PER_SEC instead of human-misreadable digits. r?dholbert
Attachment #8735336 - Attachment description: MozReview Request: Bug 1256503 - Part 2: Introduce addDivAndAnimate function checking animation duration is longer than 100s to avoid intermittent test failrues. r?birtles. → MozReview Request: Bug 1256503 - Part 2: Introduce addDivAndAnimate function checking animation duration is longer than 100s to avoid intermittent test failrues. r?dholbert
Attachment #8735335 - Flags: review?(bbirtles) → review?(dholbert)
Attachment #8735336 - Flags: review?(bbirtles) → review?(dholbert)
Comment on attachment 8735336 [details]
MozReview Request: Bug 1256503 - Part 3: Introduce addDivAndAnimate function checking animation duration is longer than 100s to avoid intermittent test failures. r?dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42743/diff/1-2/
Daniel, could you please review these patch?  I thought I was requesting to you but I wrote Brian's name in the commit messages as usual!

These patches no longer depends on bug 1255710.

Thanks!
Comment on attachment 8735335 [details]
MozReview Request: Bug 1256503 - Part 1: Use MS_PER_SEC instead of human-misreadable digits in animation tests. r?dholbert

https://reviewboard.mozilla.org/r/42741/#review39219

::: dom/animation/test/chrome/test_animate_xrays.html:22
(Diff revision 2)
>  var win = document.getElementById('iframe').contentWindow;
>  
>  async_test(function(t) {
>    window.addEventListener('load', t.step_func(function() {
>      var target = win.document.getElementById('target');
> -    var anim = target.animate({ opacity: [ 0, 1 ] }, 2000);
> +    var anim = target.animate({ opacity: [ 0, 1 ] }, 100 * MS_PER_SEC);

This line is changing an animation from having a 2-sec duration to having a 100-sec duration -- i.e. it's changing logic/behavior.

Ideally I'd prefer if we split this change (and any other changes of the sort) into its own "part 3" fixup-patch, so that this first patch can just be *purely* doing a search-and-replace with no behavior changes.  (So, this first patch should leave this line as "2 * MS_PER_SEC", and then a later patch would change it to 100 * MS_PER_SEC.)

(Generally it's best not to mix behavior/logic-impacting changes into a mass refactoring patch which is supposed to have no effect on behavior.)

::: dom/animation/test/chrome/test_animation_observers.html:1534
(Diff revision 2)
>  
>      yield await_frame();
>      assert_records([{ added: [anim], changed: [], removed: [] }],
>                     "records after animation is added");
>  
> -    anim.effect.timing.duration = 10000;
> +    anim.effect.timing.duration = 10 * MS_PER_SEC;

This duration (10 seconds) seems to violate your "Shorter durartion than 100s" prohibition, in the code-comment at the top of this patch.

Does this need to change? (Maybe in the same "part 3" patch that I suggested above, which would contain actual duration-increasing changes)

::: dom/animation/test/chrome/test_animation_observers.html:1539
(Diff revision 2)
> -    anim.effect.timing.duration = 10000;
> +    anim.effect.timing.duration = 10 * MS_PER_SEC;
>      yield await_frame();
>      assert_records([{ added: [], changed: [anim], removed: [] }],
>                     "records after duration is changed");
>  
> -    anim.effect.timing.duration = 10000;
> +    anim.effect.timing.duration = 10 * MS_PER_SEC;

Same here. (this seems to be a duration less than 100s)

::: dom/animation/test/chrome/test_animation_properties.html:496
(Diff revision 2)
>  ];
>  
>  gTests.forEach(function(subtest) {
>    test(function(t) {
>      var div = addDiv(t);
> -    var animation = div.animate(subtest.frames, 1000);
> +    var animation = div.animate(subtest.frames, 100 * MS_PER_SEC);

As noted above, the 1-sec to 100-sec change here probably doesn't belong in this mass-refactoring patch -- it should be done in a "part 3" fixup patch.

::: dom/animation/test/testcommon.js:9
(Diff revision 2)
> +
> +/**
> + * Use this variable if you specify duration or some other properties
> + * for script animation.
> + * E.g., div.animate({ opacity: [0, 1] }, 100 * MS_PER_SEC);
> + * NOTE: Shorter duration than 100s will cause intermittent failures.

This last statement in your documentation ("will cause intermittent failures") needs some rewording and explanation. In particular:
 (1) I notice several places that violate this rule, and I'm not sure which (if any) are actually-fine. For example, this patch has the following in one piece of contextual code, to cancel an animation: `e.style.animationDuration = "0.1s";`  Your documentation here says that this "*will* cause intermittent failures".  So: please update the documentation to be more specific about what you're warning against here.  (Maybe you mean to say something like: "If an animation duration isn't intended to complete during a testharness.js test, then its duration must be at least 100s, or it risks causing intermittent failures"?)

 (2) Why is 100s the magic number? Is it some testharness.js timeout value? Please include an explanation for why you're warning about that particular number here.
Attachment #8735335 - Flags: review?(dholbert)
Comment on attachment 8735336 [details]
MozReview Request: Bug 1256503 - Part 3: Introduce addDivAndAnimate function checking animation duration is longer than 100s to avoid intermittent test failures. r?dholbert

https://reviewboard.mozilla.org/r/42743/#review39275

Firstly: regarding the code that I suggested splitting out into "part 3" in my previous comment -- I think that code *really* wants to be in "part 2", since we'll need those 100-second fixes before we can take this patch here.  So this patch here (the one I'm reviewing in this comment) should really be numbered "part 3".

::: dom/animation/test/chrome/test_animation_property_state.html:220
(Diff revision 2)
>  
>  gAnimationsTests.forEach(function(subtest) {
>    promise_test(function(t) {
> -    var div = addDiv(t, { class: 'compositable' });
> -    var animation = div.animate(subtest.frames, 100 * MS_PER_SEC);
> +    var animation = addDivAndAnimate(t,
> +      { class: 'compositable' },
> +      subtest.frames, 100 * MS_PER_SEC);

Sorry to nitpick but: I find this extreme-deindentation-on-second-line-of-a-function-call really hard to read (and I don't think it's endorsed in our coding style anywhere).

In some cases, this sort of pattern is unavoidable because the function-name is reallyreallylong and the only alternative would be to have extremely long (well over 80 characters) lines. But I don't think that's the case here. Seems like this should just be:

      var animation = addDivAndAnimate(t,
                                       { class: 'compositable' },
                                       subtest.frames, 100 * MS_PER_SEC);

And similar thorughout the rest of this patch.

(In some cases you might need to put "100 * MS_PER_SEC" on its own line to avoid going over 80 characters, so please do that in those cases.)

::: dom/animation/test/testcommon.js:31
(Diff revision 2)
> +  if (typeof options === 'object') {
> +    assert_greater_than_equal(options.duration, 100 * MS_PER_SEC,
> +        'Animation duration should be greater than 100s');
> +  } else {
> +    assert_greater_than_equal(options, 100 * MS_PER_SEC,
> +        'Animation duration should be greater than 100s');

Please use only a single assertion, and just condition on the thing you're asserting about. So:

    let animDur = (typeof options === 'object') ?
      options.duration : options;
    assert_greater_than_equal(animDur, ...);

That's more maintainable, if we want to change the assertion text or the magic 100-second threshold.

Also, please including an explanation here (either in the assertion message or in a code-comment alongside the assertion) about why 100s is a relevant threshold.  (Per my previous comment, you should also explain it in your documentation for the other method -- but you should hint at it here as well, because someone who's looking at this assertion-failure won't necessarily have seen your documentation for that other method.)
Attachment #8735336 - Flags: review?(dholbert)
Both of short animation duration in synchronous tests and checking
state of the animation immediately after creating the animation synchronousely
do not matter practically but we should use longer duration to avoid
spreading the short duration by copy-and-paste.

100s is a preferable duration since it's long enough but shorter than
our test frame work timeout (330s).

Review commit: https://reviewboard.mozilla.org/r/42907/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42907/
Attachment #8735336 - Attachment description: MozReview Request: Bug 1256503 - Part 2: Introduce addDivAndAnimate function checking animation duration is longer than 100s to avoid intermittent test failrues. r?dholbert → MozReview Request: Bug 1256503 - Part 3: Introduce addDivAndAnimate function checking animation duration is longer than 100s to avoid intermittent test failrues. r?dholbert
Attachment #8735679 - Flags: review?(dholbert)
Attachment #8735335 - Flags: review?(dholbert)
Attachment #8735336 - Flags: review?(dholbert)
Comment on attachment 8735335 [details]
MozReview Request: Bug 1256503 - Part 1: Use MS_PER_SEC instead of human-misreadable digits in animation tests. r?dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42741/diff/2-3/
Comment on attachment 8735336 [details]
MozReview Request: Bug 1256503 - Part 3: Introduce addDivAndAnimate function checking animation duration is longer than 100s to avoid intermittent test failures. r?dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42743/diff/2-3/
https://reviewboard.mozilla.org/r/42741/#review39219

> This line is changing an animation from having a 2-sec duration to having a 100-sec duration -- i.e. it's changing logic/behavior.
> 
> Ideally I'd prefer if we split this change (and any other changes of the sort) into its own "part 3" fixup-patch, so that this first patch can just be *purely* doing a search-and-replace with no behavior changes.  (So, this first patch should leave this line as "2 * MS_PER_SEC", and then a later patch would change it to 100 * MS_PER_SEC.)
> 
> (Generally it's best not to mix behavior/logic-impacting changes into a mass refactoring patch which is supposed to have no effect on behavior.)

Thanks!  These changes has been moved into a new part 2 patch.

> This duration (10 seconds) seems to violate your "Shorter durartion than 100s" prohibition, in the code-comment at the top of this patch.
> 
> Does this need to change? (Maybe in the same "part 3" patch that I suggested above, which would contain actual duration-increasing changes)

Good catch!  Yes, I wanted to change this too.  My intention in this bug is to avoid using those shorter durations now and from now on.  I DO want to avoid that someone will use those short duration unintentionaly, i.e. copying it or something.  This change has been done in the new part 2 patch as well.

> This last statement in your documentation ("will cause intermittent failures") needs some rewording and explanation. In particular:
>  (1) I notice several places that violate this rule, and I'm not sure which (if any) are actually-fine. For example, this patch has the following in one piece of contextual code, to cancel an animation: `e.style.animationDuration = "0.1s";`  Your documentation here says that this "*will* cause intermittent failures".  So: please update the documentation to be more specific about what you're warning against here.  (Maybe you mean to say something like: "If an animation duration isn't intended to complete during a testharness.js test, then its duration must be at least 100s, or it risks causing intermittent failures"?)
> 
>  (2) Why is 100s the magic number? Is it some testharness.js timeout value? Please include an explanation for why you're warning about that particular number here.

Thanks! The description for the canceling case is added in this patch.  The explanation aboud the magic number '100s' will be in the commit message in the new part 2 patch.
https://reviewboard.mozilla.org/r/42743/#review39275

> Please use only a single assertion, and just condition on the thing you're asserting about. So:
> 
>     let animDur = (typeof options === 'object') ?
>       options.duration : options;
>     assert_greater_than_equal(animDur, ...);
> 
> That's more maintainable, if we want to change the assertion text or the magic 100-second threshold.
> 
> Also, please including an explanation here (either in the assertion message or in a code-comment alongside the assertion) about why 100s is a relevant threshold.  (Per my previous comment, you should also explain it in your documentation for the other method -- but you should hint at it here as well, because someone who's looking at this assertion-failure won't necessarily have seen your documentation for that other method.)

Added explanations as a comment and added also as the assertion messaage as well.

Thanks for sophisticated reviews!  I will learn a lot.
Attachment #8735335 - Flags: review?(dholbert) → review+
Comment on attachment 8735335 [details]
MozReview Request: Bug 1256503 - Part 1: Use MS_PER_SEC instead of human-misreadable digits in animation tests. r?dholbert

https://reviewboard.mozilla.org/r/42741/#review39617

Thanks! r=me with nits addressed.

Firstly: the commit message ("Part 1: Use MS_PER_SEC instead of human-misreadable digits.") needs to have "in animation tests" added at the end of it, to be clearer about its scope & what it's doing.

::: dom/animation/test/chrome/test_running_on_compositor.html:362
(Diff revision 3)
>  
>    return animation.ready.then(t.step_func(function() {
>      assert_equals(animation.isRunningOnCompositor, omtaEnabled,
>        'Animation reports that it is running on the compositor');
>  
> -    animation.effect.timing.endDelay = -200000; // -200s
> +    animation.effect.timing.endDelay = -200* MS_PER_SEC;

Nit: add space between `-200` and `*` here.

::: dom/animation/test/testcommon.js:5
(Diff revision 3)
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +
> +/**

This is adding an extra blank line at the beginning here; probably remove that.

::: dom/animation/test/testcommon.js:13
(Diff revision 3)
> + * E.g., div.animate({ opacity: [0, 1] }, 100 * MS_PER_SEC);
> + *
> + * NOTE: Creating animations with short duration may cause intermittent
> + * failures in asynchronous test. For example, the short duration animation
> + * might be finished when animation.ready has been fulfilled because of slow
> + * platforms or busyness of the main-thread.

Nit: drop the hyphen from "main-thread" -- should just be "main thread".

(English generally hyphenates multi-word nouns like "main thread" when they're being used as a descriptor/adjective, as in e.g. "main-thread animations".  But we don't tend to hyphenate these phrases when they're simply being used as nouns, as in the phrase "on the main thread".)
Comment on attachment 8735679 [details]
MozReview Request: Bug 1256503 - Part 2: Increase some test animation durations to be at least 100 seconds. r?dholbert

https://reviewboard.mozilla.org/r/42907/#review39635

This patch looks good, but the commit message is a bit hard to understand right now. I'd suggest rewriting it along these lines:
/////
Bug 1256503 - Part 2: Increase some test animation durations to be at least 100 seconds. r=dholbert

In some of these cases, this increase isn't strictly necessary, because we only state immediately after creating the animation, before it could have completed (regardless of its duration). Still: we should consistently use long durations for any animations that aren't expected to complete during the test run, because short durations might accidentally get copypasted into new tests where they might cause intermittent failures.
Attachment #8735679 - Flags: review?(dholbert) → review+
> In some of these cases, this increase isn't strictly necessary, because we only state immediately 

Sorry, I dropped a word there -- I meant to say "we only *check* state"
Comment on attachment 8735336 [details]
MozReview Request: Bug 1256503 - Part 3: Introduce addDivAndAnimate function checking animation duration is longer than 100s to avoid intermittent test failures. r?dholbert

https://reviewboard.mozilla.org/r/42743/#review39639

Commit message nits:
>Bug 1256503 - Part 3: Introduce addDivAndAnimate function checking animation duration is longer than 100s to avoid intermittent test failrues. r?dholbert

Typo: s/failrues/failures/

> There are still raw div.animate() in test_animation_performance_warning.html
> because there needs to be created a couple of animations on the same element.

I'd suggest just getting rid of this extended commit message.

It wasn't initially clear what it was trying to say, and it also implies that "div.animate()" calls *only* remain in that one test (test_animation_performance_warning.html) -- and that's not at all the case.  There are plenty more div.animate calls in other tests, as shown by a quick `grep -r ".animate(" dom/animation/test` command in my terminal.

::: dom/animation/test/chrome/test_animation_property_state.html:729
(Diff revision 3)
>      }));
>    }, 'transform of nsIFrame with SVG transform');
>  
>    promise_test(function(t) {
> -    var div = addDiv(t, { class: 'compositable',
> -                          style: 'animation: fade 100s' });
> +    var div = addDiv(t,
> +                     { class: 'compositable', style: 'animation: fade 100s' });

This change seems to be purely a whitespace change, and IMO it makes this a bit less readable.

Please either revert this change, or add back the newline before "style" so that we have the attributes listed on separate lines again, e.g.

    var div = addDiv(t,
                     { class: 'compositable',
                       style: 'animation: fade 100s' });

::: dom/animation/test/chrome/test_running_on_compositor.html:290
(Diff revision 3)
>  }, 'isRunningOnCompositor is true when a property that would otherwise block ' +
>     'running on the compositor is overridden in the CSS cascade');
>  
>  promise_test(function(t) {
> -  var div = addDiv(t);
> -  var animation = div.animate({ opacity: [ 0, 1 ] }, 200 * MS_PER_SEC);
> +  var animation = addDivAndAnimate(t,
> +                                   null,

This hardcoded "null" is a bit confusing here. Your documentation says this is supposed to be "A dictionary object with attribute names and values[...]". 
 
Could you pass in "{}" instead of "null"?  That'd be more readable, and it'd fit your documentation better, and it'd make it easier to compare these callsites to other other callsites that pass nonempty dictionaries for this arg (like "{ class: 'compositable' }").

(This applies to several calls in this file.)

::: dom/animation/test/testcommon.js:20
(Diff revision 3)
>   * if you don't want to cancel the animation, consider using longer duration.
>   */
>  const MS_PER_SEC = 1000;
>  
>  /**
> + * Appends a div to the document body and create an animation on the div.

"Appends...and create" is a typo (mixed tenses).

It should say "and creates" (with an "s").

::: dom/animation/test/testcommon.js:24
(Diff revision 3)
>  /**
> + * Appends a div to the document body and create an animation on the div.
> + * NOTE: This function asserts when trying to create animations with durations
> + * shorter than 100s because the shorter duration may cause intermittent
> + * failures.  If you are not sure how long it is suitable, use 100s, it's
> + * longer enough but shorter than our test frame work timeout (330s).

3 typos:

(1) Replace the comma after "use 100s" with a semicolon or a hyphen.
(2) s/longer/long/  (in "long enough")
(3) s/frame work/framework/

::: dom/animation/test/testcommon.js:31
(Diff revision 3)
> + *
> + * @param t  The testharness.js Test object. If provided, this will be used
> + *           to register a cleanup callback to remove the div when the test
> + *           finishes.
> + *
> + * @param attrs  A dictionary object with attribute names and values to set on

Consider dropping the blank line between @param t and @param attrs documentation here. (There's no blank line between the remaining args, so this one seems out of place.)

::: dom/animation/test/testcommon.js:42
(Diff revision 3)
> +  let animDur = (typeof options === 'object') ?
> +    options.duration : options;
> +  assert_greater_than_equal(animDur, 100 * MS_PER_SEC,
> +      'Animation duration should be greater than 100s otherwise ' +
> +      'there are some cases that the animation is finished immediately ' +
> +      'after creating it due to busyness of the main-thread or other factors');

This assertion message is a bit hard to read/understand. (In particular, it's not immediately clear that it's really sanity-checking *the way this method is called from test code*, vs. sanity-checking Gecko behavior like most of our assert_whatever invocations.  The current message-text sounds a bit like it could just be asserting that an animation duration ended up being what we expected it to be.)


Consider rewriting as something like the following to make the intention clearer:
      "Clients of this addDivAndAnimate API must request a duration " +
      "of at least 100s, to avoid intermittent failures from e.g." +
      "the main thread being busy for an extended period".
Attachment #8735336 - Flags: review?(dholbert) → review+
Comment on attachment 8735335 [details]
MozReview Request: Bug 1256503 - Part 1: Use MS_PER_SEC instead of human-misreadable digits in animation tests. r?dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42741/diff/3-4/
Attachment #8735335 - Attachment description: MozReview Request: Bug 1256503 - Part 1: Use MS_PER_SEC instead of human-misreadable digits. r?dholbert → MozReview Request: Bug 1256503 - Part 1: Use MS_PER_SEC instead of human-misreadable digits in animation tests. r?dholbert
Attachment #8735679 - Attachment description: MozReview Request: Bug 1256503 - Part 2: Use longer duration. r?dholbert → MozReview Request: Bug 1256503 - Part 2: Increase some test animation durations to be at least 100 seconds. r?dholbert
Attachment #8735336 - Attachment description: MozReview Request: Bug 1256503 - Part 3: Introduce addDivAndAnimate function checking animation duration is longer than 100s to avoid intermittent test failrues. r?dholbert → MozReview Request: Bug 1256503 - Part 3: Introduce addDivAndAnimate function checking animation duration is longer than 100s to avoid intermittent test failures. r?dholbert
Comment on attachment 8735679 [details]
MozReview Request: Bug 1256503 - Part 2: Increase some test animation durations to be at least 100 seconds. r?dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42907/diff/1-2/
Comment on attachment 8735336 [details]
MozReview Request: Bug 1256503 - Part 3: Introduce addDivAndAnimate function checking animation duration is longer than 100s to avoid intermittent test failures. r?dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42743/diff/3-4/
https://reviewboard.mozilla.org/r/42741/#review39617

> Nit: drop the hyphen from "main-thread" -- should just be "main thread".
> 
> (English generally hyphenates multi-word nouns like "main thread" when they're being used as a descriptor/adjective, as in e.g. "main-thread animations".  But we don't tend to hyphenate these phrases when they're simply being used as nouns, as in the phrase "on the main thread".)

Thanks!  Your explanation is very helpful!
You need to log in before you can comment on or make changes to this bug.