Bug 193065

Summary: 'TestGroup.commitSetsFromTestGroupsAndMeasurementSet' should also return commit sets from test groups.
Product: WebKit Reporter: dewei_zhu
Component: New BugsAssignee: dewei_zhu
Status: NEW    
Severity: Normal CC: dewei_zhu, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch dewei_zhu: review?

dewei_zhu
Reported 2019-01-01 03:11:43 PST
'TestGroup.commitSetsFromTestGroupsAndMeasurementSet' should also return commit sets from test groups.
Attachments
Patch (10.35 KB, patch)
2019-01-01 03:13 PST, dewei_zhu 		dewei_zhu: review?
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
dewei_zhu
Comment 1 2019-01-01 03:13:29 PST
Created attachment 358170 [details] Patch
Ryosuke Niwa
Comment 2 2019-01-10 20:51:36 PST
Comment on attachment 358170 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358170&action=review > Websites/perf.webkit.org/public/v3/models/analysis-task.js:193 > + const measurementSets = Array.from(series.viewBetweenPoints(startPoint, endPoint)).map((point) => point.commitSet()); It's wrong to call this measurementSets since it's not a MeasurementSet. Maybe commitSetsInMeasurementSet? > Websites/perf.webkit.org/public/v3/models/analysis-task.js:194 > + return [...allCommitSetsInTask, ...measurementSets]; Are we sure each commit in allCommitSetsInTask and measurementSets are unique? If not, maybe we should add all commit sets to a single set first. Also, adding tings to a set is probably not the right way to do this comparison given "equals" can return true for two distinct commit set objects...
dewei_zhu
Comment 3 2019-01-18 18:03:34 PST
Comment on attachment 358170 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358170&action=review >> Websites/perf.webkit.org/public/v3/models/analysis-task.js:194 >> + return [...allCommitSetsInTask, ...measurementSets]; > > Are we sure each commit in allCommitSetsInTask and measurementSets are unique? > If not, maybe we should add all commit sets to a single set first. > Also, adding tings to a set is probably not the right way to do this comparison given > "equals" can return true for two distinct commit set objects... It will be done by CommitSetRangeBisector._orderCommitSetsByTimeAndOrderThenDeduplicate later. Do we want to dedupe here?
Ryosuke Niwa
Comment 4 2019-01-22 21:49:24 PST
(In reply to dewei_zhu from comment #3) > Comment on attachment 358170 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=358170&action=review > > >> Websites/perf.webkit.org/public/v3/models/analysis-task.js:194 > >> + return [...allCommitSetsInTask, ...measurementSets]; > > > > Are we sure each commit in allCommitSetsInTask and measurementSets are unique? > > If not, maybe we should add all commit sets to a single set first. > > Also, adding tings to a set is probably not the right way to do this comparison given > > "equals" can return true for two distinct commit set objects... > > It will be done by > CommitSetRangeBisector._orderCommitSetsByTimeAndOrderThenDeduplicate later. > Do we want to dedupe here? I see. No need to unique it here then.
Note You need to log in before you can comment on or make changes to this bug.