[webkit-reviews] review granted: [Bug 223886] [perf dashboard] Add sequential mode for perf dashboard A/B testing. : [Attachment 429490] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 223886: [perf dashboard] Add sequential mode for perf dashboard A/B
testing.
https://bugs.webkit.org/show_bug.cgi?id=223886

Attachment 429490: Patch

https://bugs.webkit.org/attachment.cgi?id=429490&action=review




--- Comment #34 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 429490
  --> https://bugs.webkit.org/attachment.cgi?id=429490
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429490&action=review

> Websites/perf.webkit.org/public/v3/models/test-group.js:348
> +	   const retryLimit = maxRetryFactor * this.initialRepetitionCount();

Now I notice that this isn't really retryLimit but more like repetitionLimit so
we maybe we wanna do that rename.

> Websites/perf.webkit.org/public/v3/models/test-group.js:365
> +	   const retryCount = this.requestedCommitSets().reduce(
> +	       (currentMin, commitSet) =>
Math.min(Math.max(Math.floor(retryLimit -
this.repetitionCountForCommitSet(commitSet)), 0), currentMin),
additionalRepetitionNeeded);

I guess this is more defensive code but shouldn't repetition count for all
commit sets same in alternating group?
If so, maybe we can just do this math with the first commit set and assert that
the repetition counts for all commit sets are same separately.

> Websites/perf.webkit.org/public/v3/models/test-group.js:374
> +	       return {retryCount: 0, stopFutureRetries:
!hasUnfinishedBuildRequest};

Oh nice! This is a lot cleaner.

> Websites/perf.webkit.org/public/v3/models/test-group.js:400
> +	       const retryCount = Math.min(Math.max(Math.floor(retryLimit -
this.repetitionCountForCommitSet(commitSet)), 0), additionalRepetition);
> +	       if (!retryCount)

Maybe we can get rid of Math.max and just check retryCount <= 0 here?
It's hard to read the nested min/max/floor right now.
I guess the same comment applies to the math in
_createAlternatingRetriesForTestGroup as well.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:10
> -    constructor(config, remote, buildbotRemote, workerInfo, logger)
> +    constructor(config, remote, buildbotRemote, workerInfo, maxRetryFactor,
logger)

Why not maxRetryFactor make be a part of config instead of an argument to
sync-buildbot.js?
We probably want to store this retry factor thing in the server side in the
future though.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:161
> +	   const allRequestsFailedForPreviousCommitSet =
testGroup.requestsForCommitSet(previousCommitSet).every(request =>
!request.isTest() || request.hasFailed());
> +	   if (allRequestsFailedForPreviousCommitSet)

Maybe allTestsFailedForPreviousCommitSet instead?
Since we're all talking about requests, the fact we want to know tests might be
more relevant than "request" part.