[Webkit-unassigned] [Bug 183888] Add a bisect button to automatically schedule bisecting A/B tasks.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=183888

--- Comment #5 from dewei_zhu at apple.com ---
Comment on attachment 336889
  --> https://bugs.webkit.org/attachment.cgi?id=336889
Patch

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

>> Websites/perf.webkit.org/public/v3/models/analysis-task.js:175
>> +        const allTestGroupsInTask = TestGroup.findAllByTask(this.id());
> 
> What guarantees that we've already fetched all the test groups?

It only works for the UI part. I think I need to use fetchForTask instead.

>> Websites/perf.webkit.org/public/v3/models/analysis-task.js:191
>> +        try {
> 
> I don't really use try-catch elsewhere.
> I'm not certain if it's a good idea to use it here either because it can swallow unexpected exceptions.

When will those error eventually get caught?

>> Websites/perf.webkit.org/public/v3/models/analysis-task.js:210
>> +            throw 'Cannot bisect on a range with root/patch/owned commit';
> 
> Is the idea that we'd like to implement the MVP and expand it later?
> We certainly want to be able to support this in the future, right?

Yes. Maybe I should abstract all those functions into a module. Essentially what we want to do here are:
1. define a bisection range
2. find available commitSets (testable configurations)
3. invoke an algorithm to find the bisection point(s), this algorithm also defines what types of commitSet it can process.

For step 2, currently, we only choose the commitSet either exists in charts(measurementSet) or test groups (CommitSet), but in the future, it is possible that we can get more available binaries.
For step 3, current bisection algorithm requires CommitSet must not have root/patch/owned commits, but this can be changed if more advanced bisection algorithm is developed.

>> Websites/perf.webkit.org/public/v3/models/commit-set.js:169
>> +        secondCommitSet.repositories().forEach((repository) => allRepositories.add(repository));
> 
> Why not just new Set([...a, ...b])?

Nice, I didn't know this.

>> Websites/perf.webkit.org/public/v3/models/commit-set.js:186
>> +                nameParts.push(`${repository.name()}: Patch-${firstPatch.id()} - Patch-${secondPatch.id()}`);
> 
> I don't think it makes sense to expose the attachment ID like this. We don't do elsewhere in the UI.
> If we're concerned about the length of the name, then we can just diff the name with the maximum length of name to show.
> e.g. if we had WebKit-WebComponents-A.patch and WebKit-WebComponents-B.patch, we can just show:
> WebKit: "...A.patch" - "...B.patch"

What if two different patch have the same name?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180331/eed7bc4e/attachment-0001.html>