Add a bisect button to automatically schedule bisecting A/B tasks.
Created attachment 336260 [details] Patch
Created attachment 336818 [details] Patch
Created attachment 336889 [details] Patch
Comment on attachment 336889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336889&action=review Here are my comments half way through the 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? > 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. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:201 > + static async performBinaryBisect(commitSetInTestGroup, allCommitSetsInTask, selectBisectionPointStrategy) First off, "bisection" is a binary search by definition". "Bi" means two. Second off, this isn't performing bisection itself. It's creating a new test group to bisect. So I think we need to rename this to something like createTestGroupForBisection. Finally, the argument to this function doesn't need to say selectBisectionPointStrategy. selectBisectionPoint would suffice. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:204 > + throw 'Can only bisect a test group with 2 commit sets'; We can simply return this as a return value in the case we had an error, and null otherwise. This way, if this function throws an exception (unexpectedly), it would be easier to debug. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:206 > + const commitSetSortByLatestCommitTime = (oneCommitSet, anotherCommitSet) => oneCommitSet.latestCommitTime() - anotherCommitSet.latestCommitTime(); Does this fallback to build time for "system" graphs? > Websites/perf.webkit.org/public/v3/models/analysis-task.js:210 > + if (!allSimpleCommitSets) > + 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? > Websites/perf.webkit.org/public/v3/models/analysis-task.js:242 > + static async selectBisectionPointWithSimilarRevisionDistance(uniqueSortedCommitSets) Nit: sortedUniqueCommitSets. It's unclear what these commit sets are for. I think the idea here is to pick one commit set out of them? If so, maybe orderedCommitSet might sufficient. I don't think "unique" adds much value here. > Websites/perf.webkit.org/public/v3/models/commit-set.js:91 > + isCommitSetWithoutRootPatchOrOwnedCommit() { Nit: { should be on the new line. Since this is a method on commit set, there is no need to say commit say. How about containsRootPatchOrOwnedCommit and negate the meaning. It's usually better to have a function which tests the positive condition than a negative condition because then we end up writing code like !isCommitSetWithoutRootPatchOrOwnedCommit, which is harder to read. > Websites/perf.webkit.org/public/v3/models/commit-set.js:103 > + if (this.requiresBuildForRepository(repository)) > + return false; Why are we checking this condition? > Websites/perf.webkit.org/public/v3/models/commit-set.js:169 > + const allRepositories = new Set(firstCommitSet.repositories()); > + secondCommitSet.repositories().forEach((repository) => allRepositories.add(repository)); Why not just new Set([...a, ...b])? > Websites/perf.webkit.org/public/v3/models/commit-set.js:170 > + let nameParts = []; This should be const. > Websites/perf.webkit.org/public/v3/models/commit-set.js:186 > + if (firstCommit == secondCommit && firstPatch != secondPatch) > + 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" > Websites/perf.webkit.org/public/v3/models/commit-set.js:189 > + nameParts.push(`${repository.name()}: ${firstCommit.label()}+Patch-${firstPatch.id()} - ${secondCommit.label()}+Patch-${secondPatch.id()}`); I think "r124 with a.patch" reads better than "r124+Patch a.patch". > Websites/perf.webkit.org/public/v3/models/commit-set.js:198 > + const leftRootFileDescription = uniqueInFirstCommitSet.map((rootFile) => rootFile.id()).join(', '); > + const rightRootFileDescription = uniqueInSecondCommitSet.map((rootFile) => rootFile.id()).join(', '); Again, I don't think it makes sense to use attachment IDs.
Comment on attachment 336889 [details] 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?
(In reply to dewei_zhu from comment #5) > Comment on attachment 336889 [details] > 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. Okay. Do we cache the results there? We should make sure we don't end up fetching it over & over. > >> 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? The point is that it won't be caught and shows up on WebInspector. > >> 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? We should probably add the file size, uploaded date, etc... This seems like an unlikely scenario though. It would be confusing for humans anyway.
Comment on attachment 336889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336889&action=review >>>> 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? > > We should probably add the file size, uploaded date, etc... This seems like an unlikely scenario though. It would be confusing for humans anyway. How about we use id as a fallback when we got two identical names?
(In reply to dewei_zhu from comment #7) > Comment on attachment 336889 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=336889&action=review > > >>>> 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? > > > > We should probably add the file size, uploaded date, etc... This seems like an unlikely scenario though. It would be confusing for humans anyway. > > How about we use id as a fallback when we got two identical names? I don't think we should ever use a file ID as a fallback. That's just a random number we generate as far as the user is concerned. Alternatively, we can automatically add (1), (2), etc... as we do for test groups but is this really a common issue? If it is, we should consider letting people modify the file name instead.
Created attachment 337249 [details] Patch
Comment on attachment 337249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337249&action=review > Websites/perf.webkit.org/unit-tests/commit-set-tests.js:199 > + Will remove this extra line.
Comment on attachment 337249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337249&action=review > Websites/perf.webkit.org/unit-tests/commit-set-tests.js:256 > + assert.equal(CommitSet.describeDifferencesBetweenCommitSets(oneCommitSet(), commitSetWithAnotherWebKitCommit()), 'WebKit: webkit-commit-0 - webkit-commit-1'); We should make sure Git uses .. and SVN uses - without spaces using CommitLog.diff.
Comment on attachment 337249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337249&action=review > Websites/perf.webkit.org/public/v3/analysis-strategies.js:4 > + static async aggregateCommitSetsFromAnalysisTask(analysisTask) This should probably just be a method of AnalysisTask. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:20 > + const startPoint = series.findById(analysisTask.startMeasurementId()); > + const endPoint = series.findById(analysisTask.endMeasurementId()); > + > + const pointAfterEnd = endPoint.series.nextPoint(endPoint); Just use series.viewBetweenPoints. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:31 > + if (rangeSpecifiedByCommitSets.length != 2) { This should just be an assert since we don't currently have any test group with less or more than two commit sets. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:39 > + AnalysisStrategies.logger.error('Cannot bisect a range that is smaller than 3 commit sets'); This shouldn't emit an error message since we can legitimately hit this case when there are less than three commits between two commit sets. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:50 > + if (!commonRepositories.size) { > + AnalysisStrategies.logger.error('No common repository is found among selected commit sets'); > + return []; > + } We should just assert. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:56 > + const commitsByRepository = new Map; > + const commitSetsByConfig = new Map; I think we should extract a helper function which builds these two maps. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:64 > + try { Don't swallow error like this. With this, we don't get any backtraces when the network or some other kind of an error happens. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:68 > + AnalysisStrategies.logger.error(`Failed to fetch commits between "${firstCommit.label()}" - "${lastCommit.label()}" due to ${error}`); You need to use commit.title() instead to include the repository name. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:247 > +const AnalysisStrategies = { > + defaultStrategy: bisectRangeWithSimilarRevisionDistanceAmongExistingCommitSets, > + bisectRangeWithSimilarRevisionDistanceAmongExistingCommitSets, > + CommitSetAggregator, > + RangeSplitStrategies, > + logger: console > +}; As we discussed in person, I don't think there is much benefit to making these things runtime switchable. So there's no need to create these strategy objects/maps.
Comment on attachment 337249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337249&action=review This patch is getting really big. Why don't we split off patches to: - Add diff'ing function to CommitSet (we can start using it in analysis task pages now!) - Refactor the logic to create a new test group name for an analysis task (and use it in analysis task page!) > Websites/perf.webkit.org/public/v3/analysis-strategies.js:89 > + for (const repository of repositoriesSortedByCommitCountInRange) { As we discussed in person, we should probably sort all commits based on commit time across repositories and fallback to time-less commits' ordering. It's unfortunate that we don't necessarily have a guarantee that all commits in a given repository has commit_time set or not set. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:121 > + return [ > + {name: CommitSet.describeDifferencesBetweenCommitSets(firstCommitSet, middleCommitSet), commitSetList: [firstCommitSet, middleCommitSet]}, > + {name: CommitSet.describeDifferencesBetweenCommitSets(middleCommitSet, lastCommitSet), commitSetList: [middleCommitSet, lastCommitSet]} > + ]; This function should really just return middleCommitSet and let the caller make calls to create test groups. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:124 > + static async _sortingToolsForCommitSets(rangeSpecifiedByCommitSets, allCommitSets) What does sortingTools means in this context? I really don't like these free-standing utility functions that does a lot of magical things inside. It's a lot better if _findAndSortCommitSetsWithSameRepositorySetAsCommitSetInTargetTestGroup fetched commits logs, etc... r- because this function needs to be broken up & rewritten. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:126 > + const [oneCommitSet, anotherCommitSet] = rangeSpecifiedByCommitSets; If rangeSpecifiedByCommitSets is actually a range, these commit sets should surely be called startSet and endSet. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:127 > + if (oneCommitSet.latestCommitTime() && anotherCommitSet.latestCommitTime()) It's super unclear what the trune-ness of latestCommitTime() means. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:130 > + hasComparingKeys: (commitSet) => !! commitSet.latestCommitTime() There should be no space between !! and commitSet.latestCommitTime. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:143 > + sortingRepositories.push(repository); So we'd put a repository to sortingRepositories only if it had ordering? That seems backwards. If anything, we want them if latestCommitTime wasn't set & order is present. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:185 > + static async _findAndSortCommitSetsWithSameRepositorySetAsCommitSetInTargetTestGroup(commitSetsInTestGroup, commitSetsToBeFiltered) I think it's simpler to make this function take a single commit set, and a list of commits to filter from since we don't currently support filtering based on combined set of repositories. We can restructure the code as needs come up in the future. In general, I find that the over generalization in code is usually a bad idea because we can't predict what the future code & feature would look like until we implement them. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:190 > + if (commitSetsInTestGroup.length != 2) { > + AnalysisStrategies.logger.error('Can only bisect a test group with 2 commit sets'); > + return []; > + } Just assert this. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:192 > + const bothSimpleCommitSets = commitSetsInTestGroup.every((commitSet) => commitSet.withoutRootPatchOrOwnedCommit()); What's the point of having a local variable here? > Websites/perf.webkit.org/public/v3/analysis-strategies.js:193 > + if (!bothSimpleCommitSets) { We should simply return without emitting an error message since we don't legitimately support this. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:199 > + const sameRepositories = (commitSet) => commitSet.repositories().length == allowedRepositories.size && This should really be a helper function on CommitSet. Its name should be a verb, not noun / adjective. e.g. specifiesSameRepositories(commitSet). In fact, it's redundant to create a set of repositories out of repositories() here since CommitSet already has a bunch of sets that maps repository to a commit. You can just check those sets directly instead. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:204 > + AnalysisStrategies.logger.error('CommitSets in test group have different repositories'); > + return []; Again, we should simply exit early without an error message. In my view, these error messages need to be generated by a higher level. e.g. there is no use in showing these error messages to an end user in the front end for example. So there is no reason to generate them. Whatever backend code we write should be the one generating error messages. > Websites/perf.webkit.org/public/v3/analysis-strategies.js:208 > + const sortingTools = await RangeSplitStrategies._sortingToolsForCommitSets(commitSetsInTestGroup, commitSetsWithSameRepositories); It's super confusing that rangeSpecifiedByCommitSets is sometimes called commitSetsInTestGroup. Also, this isn't really a "range". It's simply a tuple of commit sets. Why don't we call it commitSetsToSplit? > Websites/perf.webkit.org/public/v3/models/analysis-task.js:170 > + async analysisConfigForTestGroup(testGroup) What the heck is a config? Please don't make up a word like that. Every new term we introduce is a new complexity a new maintainer of this codebase has to decipher. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:177 > + const allTestGroupsInTask = await TestGroup.fetchForTask(this.id()); > + const existingTestGroupNames = new Set; > + for (const group of allTestGroupsInTask) > + existingTestGroupNames.add(group.name()); This should really be a method on TestGroup or AnalysisTask. AnalysisTaskPage's _hasDuplicateTestGroupName should be replaced by that. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:304 > + static createNameWithoutCollision(name, hasDuplicateTestGroupName) As we discussed, we should have this in CommitSet instead. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:309 > + var nameWithNumberMatch = name.match(/(.+?)\s*\(\s*(\d+)\s*\)\s*$/); > + var number = 1; Please use const & let. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:315 > + var newName; Please use let. > Websites/perf.webkit.org/public/v3/models/commit-log.js:45 > + order() { return +this._rawData['order']; } It doesn't make any sense to force it to be a number if it's null / undefined. > Websites/perf.webkit.org/public/v3/models/commit-set.js:91 > + withoutRootPatchOrOwnedCommit() As we discussed in person, this needs to be a verb; e.g. containsRootPatchOrOwnedCommit(). > Websites/perf.webkit.org/public/v3/models/commit-set.js:164 > + static describeDifferencesBetweenCommitSets(firstCommitSet, secondCommitSet) Why not simply diff? > Websites/perf.webkit.org/public/v3/models/commit-set.js:170 > + const missingPatch = {filename: () => 'missing'}; I think "none" is a more appropriate term to be used here since it's okay for one side to be missing a patch. In fact, we'd expect that in most A/B testing. > Websites/perf.webkit.org/public/v3/models/commit-set.js:174 > + const newName = AnalysisTask.createNameWithoutCollision(name, (name) => existingNameSet.has(name)); As we discussed in person, CommitSet shouldn't rely on AnalysisTask like this. > Websites/perf.webkit.org/public/v3/models/commit-set.js:180 > + for (const repository of Array.from(allRepositories)) { We should probably use Repository.sortByNamePreferringOnesWithURL here. > Websites/perf.webkit.org/public/v3/models/commit-set.js:194 > + const nameForFirstPatch = nameGenerator(firstPatch.filename()); > + const nameForSecondPath = nameGenerator(secondPatch.filename()); We should probably limit the length of a filename and abbreviate them as needed. > Websites/perf.webkit.org/public/v3/models/commit-set.js:211 > + nameParts.push(`Roots: ${leftRootFileDescription.length ? leftRootFileDescription : 'missing'} - ${rightRootFileDescription.length ? rightRootFileDescription : 'missing'}`); I think "none" is more appropriate term to be used here since it's not like we expect roots to be always present in both sides. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:347 > + currentGroup.task().analysisConfigForTestGroup(currentGroup).then((analysisConfigs) => > + this.content('bisect-form').style.display = analysisConfigs.length ? null : 'none'); Ugh... this would mean that we'd re-comptue this every time currentGroup changes. That seems highly undesirable given it can end up fetching the data from the server, and run a whole bunch of computations. We should compute this result for each test group once and cache the result. Also, we should never directly update the style attribute asynchronously like this. This has a bug that if the current test group is changed multiple times, whichever finished the last would determine whether the button is visible or not. r- for this serious bug. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:810 > + const analysisConfigs = await this._task.analysisConfigForTestGroup(testGroup); We shouldn't be re-computing this. Just store the result somewhere the first time we computed it.
I meant that those changes should be landed separately with tests, not in a single patch.
Comment on attachment 337249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337249&action=review >> Websites/perf.webkit.org/public/v3/analysis-strategies.js:143 >> + sortingRepositories.push(repository); > > So we'd put a repository to sortingRepositories only if it had ordering? That seems backwards. > If anything, we want them if latestCommitTime wasn't set & order is present. If a commit of one repository had a latestCommitTime, we will hit the early return in line 128. >> Websites/perf.webkit.org/public/v3/analysis-strategies.js:185 >> + static async _findAndSortCommitSetsWithSameRepositorySetAsCommitSetInTargetTestGroup(commitSetsInTestGroup, commitSetsToBeFiltered) > > I think it's simpler to make this function take a single commit set, and a list of commits to filter from > since we don't currently support filtering based on combined set of repositories. > We can restructure the code as needs come up in the future. > > In general, I find that the over generalization in code is usually a bad idea because we can't predict > what the future code & feature would look like until we implement them. Line 216-218 rely on knowing the start and end commits.
Created attachment 337757 [details] Patch
Created attachment 337764 [details] Patch
Comment on attachment 337764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337764&action=review > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:49 > + const repositoriesWithCommitTime = new Set; > + const commitRangeByRepository = new Map; > + const indexForCommitWithOnlyOrderByRepository = new Map; > + const fetchingPromises = []; > + const allCommitsWithCommitTime = []; > + for (const repository of oneCommitSet.topLevelRepositories()) { > + const oneCommit = oneCommitSet.commitForRepository(repository); > + const anotherCommit = anotherCommitSet.commitForRepository(repository); > + if (oneCommit === anotherCommit) > + continue; > + > + if (oneCommit.hasCommitTime() && anotherCommit.hasCommitTime()) { > + const [startCommit, endCommit] = oneCommit.time() < anotherCommit.time() ? [oneCommit, anotherCommit] : [anotherCommit, oneCommit]; > + commitRangeByRepository.set(repository, [startCommit, endCommit]); > + fetchingPromises.push(CommitLog.fetchBetweenRevisions(repository, startCommit.revision(), endCommit.revision()).then((commits) => { > + allCommitsWithCommitTime.push(startCommit, ...commits)})); > + repositoriesWithCommitTime.add(repository); > + continue; > + } > + > + fetchingPromises.push(Promise.all([CommitLog.fetchForSingleRevision(repository, oneCommit.revision()), > + CommitLog.fetchForSingleRevision(repository, anotherCommit.revision())]).then(() => { > + if (!oneCommit.hasCommitOrder() || !anotherCommit.hasCommitOrder()) > + return; > + const [startCommit, endCommit] = oneCommit.order() < anotherCommit.order() ? [oneCommit, anotherCommit] : [anotherCommit, oneCommit]; > + commitRangeByRepository.set(repository, [startCommit, endCommit]); > + return CommitLog.fetchBetweenRevisions(repository, startCommit.revision(), endCommit.revision()).then((commits) => { > + const indexByCommit = new Map; > + commits = [startCommit, ...commits]; > + commits.forEach((commit, index) => indexByCommit.set(commit, index)); > + indexForCommitWithOnlyOrderByRepository.set(repository, indexByCommit); > + }); > + })); > + } > + await Promise.all(fetchingPromises); This does what in step 1 describes. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:54 > + let sortedCommitSets = MiddleCommitSetFinder._filterSortAndUniqueCommitSets(oneCommitSet, availableCommitSets, commitRangeByRepository, repositoriesWithCommitTime, [...indexForCommitWithOnlyOrderByRepository.keys()]); This does what in step 2 describes. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:62 > + let remainingCommitSets = MiddleCommitSetFinder._findCommitSetsClosestToMiddleOfCommitsWithTime(sortedCommitSets.slice(1, -1), repositoriesWithCommitTime, allCommitsWithCommitTime); This does what in step 3 describes. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:63 > + remainingCommitSets = MiddleCommitSetFinder._findCommitSetsClosestToMiddleOfCommitsWithOrder(remainingCommitSets, indexForCommitWithOnlyOrderByRepository); This does what in step 4 describes. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:64 > + return MiddleCommitSetFinder._chooseCommitSetClosestToMiddleOfSortedCommitSetsAmongRemainingCommitSets(remainingCommitSets, sortedCommitSets); This does what step 5 describes.
Comment on attachment 337764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337764&action=review > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:4 > +class MiddleCommitSetFinder { I'd call this CommitSetRangeBisector or CommitSetRangeSplitter instead if I were you. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:9 > + const [oneCommitSet, anotherCommitSet] = commitSetsToSplit; > + if (oneCommitSet.containsRootPatchOrOwnedCommit() || anotherCommitSet.containsRootPatchOrOwnedCommit()) "one" and "another" are terrible names. Call them "first" & "last" or "start" & "end". > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:20 > + for (const repository of oneCommitSet.topLevelRepositories()) { It would be cleaner for this to be filter((repository) => firstCommitSet.commitForRepository(repository) != lastCommitSet.commitForRepository(repository)) then map since every repository after the identity check results in exactly one promise. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:28 > + const [startCommit, endCommit] = oneCommit.time() < anotherCommit.time() ? [oneCommit, anotherCommit] : [anotherCommit, oneCommit]; > + commitRangeByRepository.set(repository, [startCommit, endCommit]); We should add a helper function to order/sort commits to CommitLog and do: commitRangeByRepository.set(repository, CommitLog.orderCommits([startCommit, endCommit])); outside of this if. We should probably also add CommitLog.hasOrdering(startCommit, endCommit) which returns false if startCommit.hasCommitTime() != endCommit.hasCommitTime() || startCommit.hasCommitOrder() != endCommit.hasCommitOrder() (the arguments should probably be called firstCommit & secondCommit). > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:30 > + allCommitsWithCommitTime.push(startCommit, ...commits)})); Missing ; and a new line after push(~). > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:31 > + repositoriesWithCommitTime.add(repository); I think it would be cleaner to split the list by filtering twice to create one list for commits with commit time, and another one with order instead of updating sets & arrays like this while iterating over repositories. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:40 > + const [startCommit, endCommit] = oneCommit.order() < anotherCommit.order() ? [oneCommit, anotherCommit] : [anotherCommit, oneCommit]; > + commitRangeByRepository.set(repository, [startCommit, endCommit]); Once you have CommitLog.hasOrdering & commitLog.orderCommits, all of this code should be shared with the code inside the above if statement. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:43 > + commits = [startCommit, ...commits]; Why do we need to create a new list?? Can't we just use -1 for startCommit's index or + 1 in forEach below? > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:67 > + static _filterSortAndUniqueCommitSets(oneCommitSetFromTestGroup, availableCommitSets, commitRangeByRepository, repositoriesWithCommitTime, repositoryWithOnlyCommitOrder) { This doesn't tell us what criteria is used to filter. Also, passing in this many arguments usually means this is not the right abstraction layer to extract as a helper function. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:68 > + const commitSetsInRange = availableCommitSets.filter((commitSet) => { This should be its own helper function, and we should directly call it in commitSetClosestToMiddleOfAllCommits > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:69 > + if (!commitSet.hasSameRepositories(oneCommitSetFromTestGroup)) I don't think this function cares whether commit set is from the test group or not. What's important here is that this is the commit set we use to filter other ones. So the variable name should reflect that. maybe filteringCommitSet? Now that think about it, it might be cleaner for this helper function to simply take an array of repositories instead. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:75 > + const [startCommit, endCommit] = commitRangeByRepository.get(repository); So commitRangeByRepository is only used by this code to filter commit sets. Why can't this store a closure which checks the filtering condition instead. i.e. why can't we simply do: if (!commitRangeByRepository.get(repository)(commit)) return false; where the map returns a closure which evaluates to true if the commit was in the range. Obviously, we should rename the variable if we did do that. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:90 > + const sortedCommitSets = commitSetsInRange.sort((commitSet0, commitSet1) => { Again, this should be a helper function on its own, and be called directly in commitSetClosestToMiddleOfAllCommits. Why are we so inconsistent in naming function arguments? We should be calling these firstCommitSet, secondCommitSet. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:115 > + const sortedUniqueCommitSets = [sortedCommitSets[0]]; > + for (let i = 1; i < sortedCommitSets.length; i += 1) { > + const previousCommitSet = sortedCommitSets[i - 1]; > + const currentCommitSet = sortedCommitSets[i]; > + if (!previousCommitSet.equals(currentCommitSet)) > + sortedUniqueCommitSets.push(currentCommitSet); > + } > + return sortedUniqueCommitSets; This can be rewritten more concisely as: commitSets.filter((currentSet, i) => !i || !currentSet.equals(commitSets[i - 1])); > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:118 > + static _findCommitSetsClosestToMiddleOfCommitsWithTime(sortedCommitSets, repositoriesWithCommitTime, allCommitsWithCommitTime) { This function isn't really finding a commit set closest to the middle. It's filtering the list of commit sets. How about _closestCommitSetsToBisectingCommitByTime? Note that we normally don't use verbs such as "find" and "get" unless they involve an out argument. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:123 > + allCommitsWithCommitTime.sort((commit0, commit1) => commit0.time() - commit1.time()) Please name these as firstCommit and secondCommit. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:126 > + const commitWithTimeToCommitSets = new Map; Since this function is named _findCommitSetsClosestToMiddleOfCommitsWithTime, we can simply call this commitToCommitSetMap. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:150 > + static _findCommitSetsClosestToMiddleOfCommitsWithOrder(remainingCommitSets, indexForCommitWithOnlyOrderByRepository) { Nit: all these functions should have { on new line. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:155 > + for (const repository of indexForCommitWithOnlyOrderByRepository.keys()) { This is pretty much the exact same code as the one in _findCommitSetsClosestToMiddleOfCommitsWithTime. We should extract this as a helper function. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:168 > + for (const commitSet of remainingCommitSets) { This is pretty much the same code we have in _findCommitSetsClosestToMiddleOfCommitsWithTime. The only difference being whether we have an enumerable set of commits vs. commit sets. We should extract a helper function for this. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:185 > + static _chooseCommitSetClosestToMiddleOfSortedCommitSetsAmongRemainingCommitSets(remainingCommitSets, sortedCommitSets) { "AmongRemaingCommitSets" doesn't add any additional information. > Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:193 > + const indexInSortedCommitSets = new Map; > + sortedCommitSets.forEach((commitSet, index) => indexInSortedCommitSets.set(commitSet, index)); Why can't simply return remainingCommitSets[Math.floor(remainingCommitSets.length / 2)]? > Websites/perf.webkit.org/public/v3/models/analysis-task.js:170 > + async middleCommitSetForTestGroup(testGroup) We shouldn't be adding a method which relies on MiddleCommitSetFinder to AnalysisTask. That's a layering violation if any. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:178 > + async aggregateCommitSets() This doesn't make it clear from where we're aggregating commit sets. How about commitSetsFromTestGroupsAndMeasurementSet? > Websites/perf.webkit.org/public/v3/models/analysis-task.js:189 > + await measurementSet.fetchBetween(this.startTime(), this.endTime()); This will throw an exception on a custom analysis task. We should check the true-ness of platform & metric. > Websites/perf.webkit.org/public/v3/models/commit-log.js:38 > + hasCommitTime() { return this._rawData['time'] > 0 && this._rawData['time'] !== null && this._rawData['time'] !== undefined; } Just use != null. In JS, null == undefined but null !== undefined. > Websites/perf.webkit.org/public/v3/models/commit-log.js:46 > + hasCommitOrder() { return this._rawData['order'] !== null && this._rawData['order'] !== undefined; } Ditto. Note 0 != null and 0 != undefined in JS. > Websites/perf.webkit.org/public/v3/models/commit-set.js:122 > + hasSameRepositories(commitSet) We should probably call this hasSameTopLevelRepositories since we don't check owned commits' repositories. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:274 > + this.dispatchAction('analyzeTestGroup', this._currentTestGroup, middleCommitSet, repetitionCount); "analyzeTestGroup" is a very generic/vague name. We should simply match the name of the part, and call it bisectTestGroup. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:289 > + await Promise.all(fetchingPromises); We can't block the updating of all UI until all commits are fetched. We need to simply enqueueToRender once this work is done. r- because this would cause a serious UI rendering delay. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:819 > + async _analyzeCurrentTestGroup(testGroup, middleCommitSet, repetitionCount) Ditto abut this generic name. This should be _bisectTestGroup since whether the test group is current or not is irrelevant for this function. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:823 > + const commitSets = [oneCommitSet, middleCommitSet, anotherCommitSet]; Why don't we make this function take a list of commit sets instead? > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:824 > + const platform = this._task.platform() || testGroup.platform(); How can task not have a platform but a test group can for a non-custom analysis task? > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:832 > + const testGroups = await TestGroup.createWithCustomConfiguration(this._task, platform, testGroup.test(), testGroupName, repetitionCount, [previousCommitSet, currentCommitSet]); Nit: Two spaces between = and await. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:835 > + alert('Failed to create a new test group: ' + error); We should probably break immediately when the attempt to create the first test group failed instead of preceding to create the second test groups after showing an error message.
Comment on attachment 337764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337764&action=review >> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:9 >> + if (oneCommitSet.containsRootPatchOrOwnedCommit() || anotherCommitSet.containsRootPatchOrOwnedCommit()) > > "one" and "another" are terrible names. > Call them "first" & "last" or "start" & "end". "first" & "last" or "start" & "end" may imply the order of commitSetsToSplit. It should be still legitimate that the commitSetToSplit is ordered in descendingly. Personally, I think first and second may be more appropriate here. >> Websites/perf.webkit.org/public/v3/middle-commit-set-finder.js:115 >> + return sortedUniqueCommitSets; > > This can be rewritten more concisely as: commitSets.filter((currentSet, i) => !i || !currentSet.equals(commitSets[i - 1])); Nice.
Created attachment 338090 [details] Patch
Created attachment 338218 [details] Patch
Comment on attachment 338218 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338218&action=review > Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:17 > + const indexForCommitWithOnlyOrderByRepository = new Map; I think we need to rename this to indexForAllTimelessCommitsWithOrderByRepository, and explain in the change log that we have to build this map for all commits as opposed to the remaining commits because we care about the distance between commits considering commits not appearing any of commit sets we have. > Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:21 > + .filter((repository) => firstCommitSet.commitForRepository(repository) !== secondCommitSet.commitForRepository(repository)); Check has ordering here as well. > Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:32 > + fetchingPromises.push(CommitLog.fetchBetweenRevisions(repository, startCommit.revision(), endCommit.revision()) And then map the result. > Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:33 > + .then((commits) => { That way, you can us await here like so: await Promise.all(topLevelRepositoriesWithCommitChange.map((repository) => { const commits = await CommitLog.fetchBetweenRevisions(repository, startCommit.revision(), endCommit.revision()); ... })); > Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:54 > + const commitSetsInRange = CommitSetRangeBisector._findCommitSetsWithinRange(firstCommitSet, availableCommitSets, commitRangeByRepository); We can just do: this._findCommitSetsWithinRange > Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:85 > + static _sortAndUniqueCommitSets(commitSets, repositoriesWithCommitTime, repositoryWithOnlyCommitOrder) This function name doesn't tell us what kind of sorting we're doing. It should be renamed to something that describes how we're sorting; e.g. orderCommitSetsByTimeThenOrder > Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:87 > + const sortedCommitSets = commitSets.sort((firstCommitSet, secondCommitSet) => { I think it's cleaner to first make the copy of commitSets. > Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:93 > + if (firstCommit.time() === secondCommit.time()) > + continue; > + return firstCommit.time() - secondCommit.time(); Just do: const diff = firstCommit.time() - secondCommit.time(); if (!diff) continue; return diff; > Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:100 > + if (firstCommit.order() === secondCommit.order()) > + continue; > + return firstCommit.order() - secondCommit.order(); Ditto. > Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:127 > + const commitOnlyOrderAvailableToCommitSets = CommitSetRangeBisector._buildCommitToCommitSetMap(indexForCommitWithOnlyOrderByRepository.keys(), remainingCommitSets); This variable name is rather long & confusing. How about commitWithOrderToCommitSets? > Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:130 > + const commits = remainingCommitSets.map((commitSet) => commitSet.commitForRepository(repository)); I think should call this one: commitsInRemainingSetsForCurrentRepository. > Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:132 > + const commitSets = commitOnlyOrderAvailableToCommitSets.get(closestCommit); Since we have so many sets & maps, we should be slightly more explicit than "commitSets". How about commitSetsContainingClosestCommit? > Websites/perf.webkit.org/public/v3/commit-set-range-bisector.js:154 > + static _findClosestCommitByIndex(indexByCommit, commits) This doesn't tell us closest to what. Maybe _findCommitClosestToMiddleIndex? > Websites/perf.webkit.org/public/v3/models/analysis-task.js:172 > + const allTestGroupsInTask = await TestGroup.fetchForTask(this.id()); I think this code should be below where measurementSet.fetchBetween happens so that the fetching could be parallelized. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:180 > + console.assert(platform); > + console.assert(metric); I think we should just return an empty list instead of asserting. > Websites/perf.webkit.org/public/v3/models/analysis-task.js:183 > + await measurementSet.fetchBetween(this.startTime(), this.endTime()); Can we have blank lines around await? > Websites/perf.webkit.org/public/v3/models/analysis-task.js:190 > + for (const point of timeSeriesView) > + allCommitSetsInTask.add(point.commitSet()); I don't think checking equality with Set is useful because MeasurementAdaptor creates a unique MeasurementCommitSet for each data point. Why don't we just map and extract each commit set instead? > Websites/perf.webkit.org/public/v3/models/commit-log.js:100 > + firstCommit.hasCommitOrder() && secondCommit.hasCommitOrder(); Nit: || should be on this line, and we should wrap this condition in parentheses as well. > Websites/perf.webkit.org/public/v3/models/commit-log.js:107 > + const firstCommitSmaller = (firstCommit.hasCommitTime() && secondCommit.hasCommitTime() > + && firstCommit.time() < secondCommit.time()) || firstCommit.order() < secondCommit.order(); This is not right. We need to use tertiary expression here: firstCommit.hasCommitTime() && secondCommit.hasCommitTime() ? firstCommit.time() < secondCommit.time() : firstCommit.order() < secondCommit.order() In fact, we can just check firstCommit.hasCommitTime() since we're asserting that two commits have an order already. Also, please add a test for this. > Websites/perf.webkit.org/public/v3/models/commit-set.js:124 > + return commitSet.repositories().length === this._repositoryToCommitMap.size && Nit: && should be on the new line. > Websites/perf.webkit.org/public/v3/models/commit-set.js:154 > + containsRootPatchOrOwnedCommit() Can we just say containsRootOrPatchOrOwnedCommit? "RootPatch" sounds like one word as opposed to two words with missing comma. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:261 > + this._middleCommitSetByTestGroup = null; I think should call this _bisectingCommitSetByTestGroup to match the terminology elsewhere. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:283 > + const testGroupIdSet = new Set(testGroups.map((testGroup) => testGroup.id())); > + const sameTestGroups = this._testGroups.length === testGroups.length && this._testGroups.every((testGroup) => testGroupIdSet.has(testGroup.id())); I don't think this is right because we're getting the filtered test groups, which is affected by their hidden state. Use TestGroup.findAllByTask instead. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:290 > + this._middleCommitSetByTestGroup = new Map; This should be reset using a lazily evaluated function. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:291 > + const fetchingPromises = testGroups.map(async (testGroup) => { We should probably just compute this for the current test group. > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:831 > + const testGroupName = CommitSet.createNameWithoutCollision(CommitSet.diff(previousCommitSet, currentCommitSet), existingTestGroupNames); Use createAndRefetchTestGroups instead since we're not gonna support custom analysis tasks here. > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:7 > +let MockModels = require('./resources/mock-v3-models.js').MockModels; > +let MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI; Use const. > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:183 > + let requests = MockRemoteAPI.inject(); Use const. > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:200 > + const promise = CommitSetRangeBisector.commitSetClosestToMiddleOfAllCommits([startCommitSet, endCommitSet], allCommitSets); > + const middleCommitSet = await promise; We don't the local variable "promise". Just await directly. > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:204 > + it('should throw exception if failed to fetch commit log', async () => { throw exception when* > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:216 > + assert.equal(error, rejectReason); Try using assert.throws instead. > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:221 > + it('should return "null" if no commit set is find other than the commit sets define the range', async () => { No commit set is found* other than the commit sets that* define > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:225 > + let promise = CommitSetRangeBisector.commitSetClosestToMiddleOfAllCommits([startCommitSet, endCommitSet], [startCommitSet, endCommitSet]); Use const. > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:232 > + repository: MockModels.osx.id(), Can we store MockModels.osx & MockModels.webkit as local variables so that we don't keep repeating? > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:291 > + it('should return bisection point closest to the middle of revision range', async () => { return bisecting* commit set* closest to the middle. > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:295 > + let promise = CommitSetRangeBisector.commitSetClosestToMiddleOfAllCommits([startCommitSet, endCommitSet], allCommitSets); Uset const. > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:298 > + const osx_fetch_request = requests.find((fetch_request) => fetch_request.url === '/api/commits/9/?precedingRevision=osx-commit-1&lastRevision=osx-commit-3'); > + const webkit_fetch_request = requests.find((fetch_request) => fetch_request.url === '/api/commits/11/?precedingRevision=webkit-commit-1&lastRevision=webkit-commit-6'); Use camelCase. > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:359 > + const middleCommitSet = await promise; > + const expectedMiddleCommitSet = allCommitSets[3]; > + assert.equal(middleCommitSet, expectedMiddleCommitSet); I don't we think we need to split this into three different lines. > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:362 > + it('should return same bisection point even when two commit sets from original commit set have inversed order', async () => { have inverted* order or inverse* order > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:366 > + let promise = CommitSetRangeBisector.commitSetClosestToMiddleOfAllCommits([endCommitSet, startCommitSet], allCommitSets); Use const. > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:427 > + const middleCommitSet = await promise; > + const expectedMiddleCommitSet = allCommitSets[3]; > + assert.equal(middleCommitSet, expectedMiddleCommitSet); Ditto about fitting all of this into a single line. > Websites/perf.webkit.org/unit-tests/commit-set-range-bisector-tests.js:525 > +}); I think we need a few more test cases. 1. The case when a subset of commit sets contain more owned repositories than others, and vice versa. 2. Cases where the order of commits in two different repositories are reversed in some repositories.
<rdar://problem/34471300>