Bug 192972 – Add UI in analysis task page to show commit testability information.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Add UI in analysis task page to show commit testability infomation.

Created attachment 357925 [details]
Patch

Created attachment 357926 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 357926 [details]
Patch

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

> Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.js:1
> +

You need to add this to index.html

> Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.js:31
> +    });

You should add a test where the test where the fetching of commits fail.
And make sure the user can still create a test.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:197
> +            this.renderReplace(this.content('comparison-testability'), this._buildTestabilityTable(this._commitSetMap['Comparison'], 'Comparison', this._invalidRevisionForRepositoryByConfiguration['Comparison']));

This line is really long. Maybe wrap it around at some point?

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:348
> +            fetchingPromises.push(CommitLog.fetchForSingleRevision(repository, revision).then(
> +                (commits) => {

Split this into a helper function so that can we use await here as we talked about it in person.
Also use Array.from & map to simplify the logic here.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:509
> +                rows.push(element('tr', element('td', `${commit.repository().name()} - ${commit.label()}: ${commit.testability()}`)));

We should be using ul & li here since this is really a list of warnings.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:-489
> -            this._updateCommitSetMap();

As we discussed in person, we should keep this function call in fact.
We should split out the work to fetch the commit sets from the server and only do that in a timer.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:153
> +
> +

Nit: two blank lines here.

Created attachment 358009 [details]
Patch

Created attachment 358048 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 358048 [details]
Patch

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

The patch mostly looks good but there are two subtle but major problems so r- for now.

> Websites/perf.webkit.org/ChangeLog:9
> +        Add UI in custom analysis task configuration and customizable test group form to
> +        show testability information.

It seems like this would fit in a single line?

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:61
> +        this._specifiedRevisions = {'Baseline': new Map, 'Comparison': new Map};
> +        this._fetchedCommits = {'Baseline': new Map, 'Comparison': new Map};
> +        this._invalidRevisionForRepositoryByConfiguration = {'Baseline': new Map, 'Comparison': new Map};

I specifically didn't do this because people had complained that changing the platform
(e.g. from MBA to MBP) shouldn't reset which revision they had selected for, say, WebKit.
So we shouldn't do this. r- because of this. Not sure why I didn't catch this earlier.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:200
> +        if (this._showComparison)
> +            this.renderReplace(this.content('comparison-testability'), this._buildTestabilityList(this._commitSetMap['Comparison'], 'Comparison',
> +                this._invalidRevisionForRepositoryByConfiguration['Comparison']));

Nit: Need { ~ } around this two line statement.
In WebKit coding guideline the number of lines refer to the number of physical lines, not the number of statements.
Because the coding guideline also prohibits having multiple statements per line,
each physical line almost always happens to end up having single statement.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:403
> +            fetchedCommits.delete(repository);

This seems too late. It's problematic that we'd have a stale commit in this._fetchedCommits until we're done fetching.
That is, the user picks revision A, we'd fetch A.
Then the user picks revision B, but until we fetch A, we'd have B in this._fetchedCommits.
I think this bug exists before this patch but since we're modifying so much of the commit fetching logic
that we might as well as fix it while we're at it.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:508
> +        for (const repository of commitSet ? commitSet.repositories() : []) {

Why don't we just return early when commitSet is undefined / null?

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:509
> +            const commit = this._fetchedCommits[configurationName].get(repository);

This isn't right. There is no guarantee that the newly set commit had already been fetched.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:516
> +        if (!entries.length)
> +            return [];

What the heck is the point of this early return??

Comment on attachment 358048 [details]
Patch

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

>> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:61
>> +        this._invalidRevisionForRepositoryByConfiguration = {'Baseline': new Map, 'Comparison': new Map};
> 
> I specifically didn't do this because people had complained that changing the platform
> (e.g. from MBA to MBP) shouldn't reset which revision they had selected for, say, WebKit.
> So we shouldn't do this. r- because of this. Not sure why I didn't catch this earlier.

OK, I misunderstood the intention. I added this logic while I was revising this patch per last review.
However, I don't think previous code works as expected because we will call `CommitLog.fetchLatestCommitForPlatform` anyway. If it successfully fetched the commits, the commitSetMap will be overwrite anyway. If the fetch returns nothing, the existing commitSetMap is not used to fill the input field and user will end up with blank input field.
Even if the fetch failed, we shouldn't use the existing CommitSetMap because the platform can be completely different OS build, (e.g. Mojave vs High Sierra).
Overall, maintaining the previous commit set doesn't seem to be helpful.

However, in order to work as expected, we need the ability to group platforms which we had some discussion earlier. If we had this grouping info, we can even change `CommitLog.fetchLatestCommitForPlatform` to `CommitLog.fetchLatestCommitForPlatformGroup`.

>> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:403
>> +            fetchedCommits.delete(repository);
> 
> This seems too late. It's problematic that we'd have a stale commit in this._fetchedCommits until we're done fetching.
> That is, the user picks revision A, we'd fetch A.
> Then the user picks revision B, but until we fetch A, we'd have B in this._fetchedCommits.
> I think this bug exists before this patch but since we're modifying so much of the commit fetching logic
> that we might as well as fix it while we're at it.

I think the right logic is remove it before fetching.
1. fetchCommits is set in two places, one is after invoking `CommitLog.fetchLatestCommitForPlatform`, the other is line 398.
2. `fetchedCommits` can either be the commit specified in `specifiedRevisions` or null if the revision is invalid.
Based on 1 & 2, I think the right logic should be remove it before fetch and set it if it fetches successfully.

>> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:509
>> +            const commit = this._fetchedCommits[configurationName].get(repository);
> 
> This isn't right. There is no guarantee that the newly set commit had already been fetched.

Per discussion around line 403, this is guaranteed.

Comment on attachment 358048 [details]
Patch

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

>>> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:403
>>> +            fetchedCommits.delete(repository);
>> 
>> This seems too late. It's problematic that we'd have a stale commit in this._fetchedCommits until we're done fetching.
>> That is, the user picks revision A, we'd fetch A.
>> Then the user picks revision B, but until we fetch A, we'd have B in this._fetchedCommits.
>> I think this bug exists before this patch but since we're modifying so much of the commit fetching logic
>> that we might as well as fix it while we're at it.
> 
> I think the right logic is remove it before fetching.
> 1. fetchCommits is set in two places, one is after invoking `CommitLog.fetchLatestCommitForPlatform`, the other is line 398.
> 2. `fetchedCommits` can either be the commit specified in `specifiedRevisions` or null if the revision is invalid.
> Based on 1 & 2, I think the right logic should be remove it before fetch and set it if it fetches successfully.

This should be changed even earlier when the specifiedRevision is changed.

Comment on attachment 358048 [details]
Patch

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

>>> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:61
>>> +        this._invalidRevisionForRepositoryByConfiguration = {'Baseline': new Map, 'Comparison': new Map};
>> 
>> I specifically didn't do this because people had complained that changing the platform
>> (e.g. from MBA to MBP) shouldn't reset which revision they had selected for, say, WebKit.
>> So we shouldn't do this. r- because of this. Not sure why I didn't catch this earlier.
> 
> OK, I misunderstood the intention. I added this logic while I was revising this patch per last review.
> However, I don't think previous code works as expected because we will call `CommitLog.fetchLatestCommitForPlatform` anyway. If it successfully fetched the commits, the commitSetMap will be overwrite anyway. If the fetch returns nothing, the existing commitSetMap is not used to fill the input field and user will end up with blank input field.
> Even if the fetch failed, we shouldn't use the existing CommitSetMap because the platform can be completely different OS build, (e.g. Mojave vs High Sierra).
> Overall, maintaining the previous commit set doesn't seem to be helpful.
> 
> However, in order to work as expected, we need the ability to group platforms which we had some discussion earlier. If we had this grouping info, we can even change `CommitLog.fetchLatestCommitForPlatform` to `CommitLog.fetchLatestCommitForPlatformGroup`.

As discussed in person, it should only clear the commit set when all revisions in all input elements are remained untouched by user.

Created attachment 358860 [details]
Patch

Created attachment 358878 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 358878 [details]
Patch

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

r=me assuming the following comments are addressed.

> Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.js:56
> +        await sleep(100);

What is this? Why are we waiting for 100ms?
We should avoid waiting for a fixed amount like this as much as possible.
If there is some kind of timeout in the UI code itself,
then we should be periodically polling to get to a good state instead of hard-coding a timeout like this.

> Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.js:65
> +        await sleep(100);

Ditto.

> Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.js:143
> +        await sleep(100);

Ditto.

> Websites/perf.webkit.org/browser-tests/custom-analysis-task-configurator-tests.js:152
> +        await sleep(100);

Ditto.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:17
> -        this._updateTriggerableLazily = new LazilyEvaluatedFunction(this._updateTriggerable.bind(this));
> +        this._invalidRevisionForRepositoryByConfiguration = {'Baseline': new Map, 'Comparison': new Map};

This variable name is pretty long. I think we can get away by simply _invalidRevisionsByConfiguration

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:62
> +            for (const [repository, commit] of this._fetchedCommits[configuration].entries()) {

We should probably extract this code to create a new Map based on this._specifiedRevisions[configuration]
as a helper function instead of repeating the same code twice.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:67
> +            this._fetchedCommits[configuration] = newFetchedCommitsForConfiguration;

We should avoid seeing it to the new map when there was no change so that the lazily evaluated function can avoid the work.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:213
> +        this.renderReplace(this.content('baseline-testability'),
> +            this._buildTestabilityList(this._commitSetMap['Baseline'], 'Baseline', this._invalidRevisionForRepositoryByConfiguration['Baseline']));

Why are we rapping the line right after the first argument?? There is a lot of space after the first argument.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:217
> +        }

We need to clear this.content('comparison-testability') when this._showComparison is false.
Note that a single analysis task configurator could be used to show multiple analysis tasks
when navigating from one analysis task to another.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:409
> +        try {

We should only wrap the call to CommitLog.fetchForSingleRevision with this try & catch.
Also, we should assert that the error we got is UnknownCommit.
Otherwise can suppress random exceptions being thrown elsewhere.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:586
> +            }, 100);

We should probably expose this 100ms delay in some constant like
CustomAnalysisTaskConfigurator.commitFetchInterval
so that the tests can use it instead of hard-coding 100ms there again.

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:149
> +        for (const commitSet of commitSets)
> +            hasCommitWithTestability |= !!commitSet.commitsWithTestability();

Should be commitSets.some(() => !!commitSet.commitsWithTestability()).

> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:156
> +                element('tr', {class: 'testability'}, `${commit.repository().name()} - ${commit.label()}: ${commit.testability()}`));

Use commit.label() instead of getting repository name & label manually?

Comment on attachment 358878 [details]
Patch

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

>> Websites/perf.webkit.org/public/v3/components/customizable-test-group-form.js:156
>> +                element('tr', {class: 'testability'}, `${commit.repository().name()} - ${commit.label()}: ${commit.testability()}`));
> 
> Use commit.label() instead of getting repository name & label manually?

You meant commit.title()?

Change is landed in r240104.

rdar://problem/44369873