Fix the task overlap check in maxExpectedQueueingTimeInSlidingWindow.

Fix the task overlap check in maxExpectedQueueingTimeInSlidingWindow.

The maxExpectedQueueingTimeInSlidingWindow function throws an exception
if the tasks provided to at as arguments overlap.

Due to floating-point precision errors the test for overlap can produce
false positive results. We used Number.EPSILON (~2e-16) to account for
precision errors when comparing the task endpoints, but the epsilon
turned out to be too small.

This patch bumps up the epsilon to 1e-3.

BUG=chromium:625701
#

Review-Url: https://codereview.chromium.org/2713043003
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/877b7469adc4b2994454cffbfc21a38454f76235

[ulan, 2017-02-24 15:13:26]

Description was changed from

==========
Fix the task overlap chekc in maxExpectedQueueingTimeInSlidingWindow.

The maxExpectedQueueingTimeInSlidingWindow function throws an exception
if the tasks provided to at as arguments overlap.

Due to floating-point precision errors the test for overlap can produce
false positive results. We used Number.EPSILON (~2e-16) to account for
precision error, but that turned out to be too small.

BUG=chromium:625701#
==========

to

==========
Fix the task overlap check in maxExpectedQueueingTimeInSlidingWindow.

The maxExpectedQueueingTimeInSlidingWindow function throws an exception
if the tasks provided to at as arguments overlap.

Due to floating-point precision errors the test for overlap can produce
false positive results. We used Number.EPSILON (~2e-16) to account for
precision errors when comparing the task endpoints, but the epsilon
turned out to be too small.

This patch bumps up the epsilon to 1e-3.

BUG=chromium:625701#
==========

[ulan, 2017-02-24 15:14:18]

ulan@chromium.org changed reviewers:
+ charliea@chromium.org

[ulan, 2017-02-24 15:14:19]

Charlie, could you please take a look?
We discussed this check in one of my previous CLs.

[charliea (OOO until 10-5), 2017-02-24 20:14:55]

Would you mind writing a short unit test that verifies this is working
correctly?

[charliea (OOO until 10-5), 2017-02-24 21:15:12]

On 2017/02/24 20:14:55, charliea wrote:
> Would you mind writing a short unit test that verifies this is working
> correctly?

(Feel free to ping me directly to take a look at this CL once this is done so
that I can approve this CL and you can continue making progress)

[ulan, 2017-02-24 21:15:18]

Added tests, ptal.

[charliea (OOO until 10-5), 2017-02-24 21:22:01]

lgtm w/ comment

https://codereview.chromium.org/2713043003/diff/20001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/estimated_input_latency_test.html (right):

https://codereview.chromium.org/2713043003/diff/20001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/estimated_input_latency_test.html:318:
assert.closeTo((100.0001 + 0.0001) / 2,
General comment: there are a bunch of different behaviors being tested here
within the same test. In Catapult, we generally prefer having a single test per
behavior as suggested by the Google best practices
(https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b...)
rather than a single test per method. However, given that this test already
exists, I guess it's better to match the style of the file rather than the style
of Catapult as a whole. At some point in the future though, these tests should
really be rewritten to look more similar to something like this:
https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/m...

[ulan, 2017-02-24 21:28:11]

https://codereview.chromium.org/2713043003/diff/20001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/estimated_input_latency_test.html (right):

https://codereview.chromium.org/2713043003/diff/20001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/estimated_input_latency_test.html:318:
assert.closeTo((100.0001 + 0.0001) / 2,
On 2017/02/24 21:22:01, charliea wrote:
> General comment: there are a bunch of different behaviors being tested here
> within the same test. In Catapult, we generally prefer having a single test
per
> behavior as suggested by the Google best practices
>
(https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b...)
> rather than a single test per method. However, given that this test already
> exists, I guess it's better to match the style of the file rather than the
style
> of Catapult as a whole. At some point in the future though, these tests should
> really be rewritten to look more similar to something like this:
>
https://cs.corp.google.com/github/catapult-project/catapult/tracing/tracing/m...

Thank you. As discussed online, I'll refactor this file in a separate CL.

[ulan, 2017-02-24 21:28:16]

The CQ bit was checked by ulan@chromium.org

[commit-bot: I haz the power, 2017-02-24 21:28:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 21:50:26]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487971696977730,
"parent_rev": "7ce5ef1fe0366acefe67bb70775739b71362bf65", "commit_rev":
"877b7469adc4b2994454cffbfc21a38454f76235"}

[commit-bot: I haz the power, 2017-02-24 21:50:30]

Description was changed from

==========
Fix the task overlap check in maxExpectedQueueingTimeInSlidingWindow.

The maxExpectedQueueingTimeInSlidingWindow function throws an exception
if the tasks provided to at as arguments overlap.

Due to floating-point precision errors the test for overlap can produce
false positive results. We used Number.EPSILON (~2e-16) to account for
precision errors when comparing the task endpoints, but the epsilon
turned out to be too small.

This patch bumps up the epsilon to 1e-3.

BUG=chromium:625701#
==========

to

==========
Fix the task overlap check in maxExpectedQueueingTimeInSlidingWindow.

The maxExpectedQueueingTimeInSlidingWindow function throws an exception
if the tasks provided to at as arguments overlap.

Due to floating-point precision errors the test for overlap can produce
false positive results. We used Number.EPSILON (~2e-16) to account for
precision errors when comparing the task endpoints, but the epsilon
turned out to be too small.

This patch bumps up the epsilon to 1e-3.

BUG=chromium:625701#

Review-Url: https://codereview.chromium.org/2713043003
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-02-24 21:50:32]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...