Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2913383005:
[Telemetry] Add temporary disabling of benchmark to story expectations. (Closed)
|
Created:
3 years, 6 months ago by rnephew (Reviews Here) Modified:
3 years, 6 months ago CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Telemetry] Add temporary disabling of benchmark to story expectations.
Before we only supported permanent disabling. This was so that tests that we
know cant run on certain platforms (tests that use android specific calls) are
not accidently run on other platforms. This adds the ability to disable failing
benchmarks on a temporary basis.
BUG=chromium:713222
Patch Set 1 #
Messages
Total messages: 17 (2 generated)
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com, perezju@chromium.org
On 2017/06/01 19:14:24, rnephew (Reviews Here) wrote: Why do we need this?
On 2017/06/01 23:31:14, nednguyen wrote: > On 2017/06/01 19:14:24, rnephew (Reviews Here) wrote: > > Why do we need this? In the 'How telemetry disables tests' document I wrote and you helped with (or Charlie did? I can't recall which at the moment). Under ShouldDisable we say 'Maybe remove'. I was operating under the assumption we still wanted to get rid of this. See https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli... for an example of it being used.
On 2017/06/02 00:58:36, rnephew (Reviews Here) wrote: > On 2017/06/01 23:31:14, nednguyen wrote: > > On 2017/06/01 19:14:24, rnephew (Reviews Here) wrote: > > > > Why do we need this? > > In the 'How telemetry disables tests' document I wrote and you helped with (or > Charlie did? I can't recall which at the moment). Under ShouldDisable we say > 'Maybe remove'. I was operating under the assumption we still wanted to get rid > of this. See > https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli... > for an example of it being used. More on why we want this. The PermanentlyDisable version will not run with --also-run-disabled-tests since its for tests that just flat out cannot run on the given platform; but this one will which will make local reproduction easier for developers for disabling that is done with the TemporarilyDisable method.
On 2017/06/02 00:59:45, rnephew (Reviews Here) wrote: > On 2017/06/02 00:58:36, rnephew (Reviews Here) wrote: > > On 2017/06/01 23:31:14, nednguyen wrote: > > > On 2017/06/01 19:14:24, rnephew (Reviews Here) wrote: > > > > > > Why do we need this? > > > > In the 'How telemetry disables tests' document I wrote and you helped with (or > > Charlie did? I can't recall which at the moment). Under ShouldDisable we say > > 'Maybe remove'. I was operating under the assumption we still wanted to get > rid > > of this. See > > > https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli... > > for an example of it being used. > > More on why we want this. The PermanentlyDisable version will not run with > --also-run-disabled-tests since its for tests that just flat out cannot run on > the given platform; but this one will which will make local reproduction easier > for developers for disabling that is done with the TemporarilyDisable method. For TemporarilyDisable test, I expect people should just use it at story level, and not at benchmark level.
On 2017/06/02 01:03:25, nednguyen wrote: > On 2017/06/02 00:59:45, rnephew (Reviews Here) wrote: > > On 2017/06/02 00:58:36, rnephew (Reviews Here) wrote: > > > On 2017/06/01 23:31:14, nednguyen wrote: > > > > On 2017/06/01 19:14:24, rnephew (Reviews Here) wrote: > > > > > > > > Why do we need this? > > > > > > In the 'How telemetry disables tests' document I wrote and you helped with > (or > > > Charlie did? I can't recall which at the moment). Under ShouldDisable we say > > > 'Maybe remove'. I was operating under the assumption we still wanted to get > > rid > > > of this. See > > > > > > https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli... > > > for an example of it being used. > > > > More on why we want this. The PermanentlyDisable version will not run with > > --also-run-disabled-tests since its for tests that just flat out cannot run on > > the given platform; but this one will which will make local reproduction > easier > > for developers for disabling that is done with the TemporarilyDisable method. > > For TemporarilyDisable test, I expect people should just use it at story level, > and not at benchmark level. So just to confirm the preferred path forward: I should scrap this CL completely, and in the other CL move the temporary story disabling back to ShouldDisable?
On 2017/06/02 01:37:52, rnephew (Reviews Here) wrote: > On 2017/06/02 01:03:25, nednguyen wrote: > > On 2017/06/02 00:59:45, rnephew (Reviews Here) wrote: > > > On 2017/06/02 00:58:36, rnephew (Reviews Here) wrote: > > > > On 2017/06/01 23:31:14, nednguyen wrote: > > > > > On 2017/06/01 19:14:24, rnephew (Reviews Here) wrote: > > > > > > > > > > Why do we need this? > > > > > > > > In the 'How telemetry disables tests' document I wrote and you helped with > > (or > > > > Charlie did? I can't recall which at the moment). Under ShouldDisable we > say > > > > 'Maybe remove'. I was operating under the assumption we still wanted to > get > > > rid > > > > of this. See > > > > > > > > > > https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli... > > > > for an example of it being used. > > > > > > More on why we want this. The PermanentlyDisable version will not run with > > > --also-run-disabled-tests since its for tests that just flat out cannot run > on > > > the given platform; but this one will which will make local reproduction > > easier > > > for developers for disabling that is done with the TemporarilyDisable > method. > > > > For TemporarilyDisable test, I expect people should just use it at story > level, > > and not at benchmark level. > > So just to confirm the preferred path forward: > I should scrap this CL completely, and in the other CL move the temporary story > disabling back to ShouldDisable? Most benchmarks that are currently disabled are due to the fact that we didn't have ways to disabling failing stories back then. I would suggest enabling them all & only disable failing stories. For benchmarks that we would refactor soon like smoothness.*, power.*, you can just use PermanentlyDisable(..) to make the work easier.
On 2017/06/02 01:45:50, nednguyen wrote: > On 2017/06/02 01:37:52, rnephew (Reviews Here) wrote: > > On 2017/06/02 01:03:25, nednguyen wrote: > > > On 2017/06/02 00:59:45, rnephew (Reviews Here) wrote: > > > > On 2017/06/02 00:58:36, rnephew (Reviews Here) wrote: > > > > > On 2017/06/01 23:31:14, nednguyen wrote: > > > > > > On 2017/06/01 19:14:24, rnephew (Reviews Here) wrote: > > > > > > > > > > > > Why do we need this? > > > > > > > > > > In the 'How telemetry disables tests' document I wrote and you helped > with > > > (or > > > > > Charlie did? I can't recall which at the moment). Under ShouldDisable we > > say > > > > > 'Maybe remove'. I was operating under the assumption we still wanted to > > get > > > > rid > > > > > of this. See > > > > > > > > > > > > > > > https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli... > > > > > for an example of it being used. > > > > > > > > More on why we want this. The PermanentlyDisable version will not run with > > > > --also-run-disabled-tests since its for tests that just flat out cannot > run > > on > > > > the given platform; but this one will which will make local reproduction > > > easier > > > > for developers for disabling that is done with the TemporarilyDisable > > method. > > > > > > For TemporarilyDisable test, I expect people should just use it at story > > level, > > > and not at benchmark level. > > > > So just to confirm the preferred path forward: > > I should scrap this CL completely, and in the other CL move the temporary > story > > disabling back to ShouldDisable? > > Most benchmarks that are currently disabled are due to the fact that we didn't > have ways to disabling failing stories back then. I would suggest enabling them > all & only disable failing stories. > > For benchmarks that we would refactor soon like smoothness.*, power.*, you can > just use PermanentlyDisable(..) to make the work easier. I think that should be done in a separate CL though, so for the current 2 cls, I'll do my plan. Then in a follow up CL get rid of ShouldDisable, look and see what starts failing and disable stories accordingly. That way I can make it more clear that failures are expected in the CL name so its less chance for confusion for the sheriffs.
On 2017/06/02 01:55:39, rnephew (Reviews Here) wrote: > On 2017/06/02 01:45:50, nednguyen wrote: > > On 2017/06/02 01:37:52, rnephew (Reviews Here) wrote: > > > On 2017/06/02 01:03:25, nednguyen wrote: > > > > On 2017/06/02 00:59:45, rnephew (Reviews Here) wrote: > > > > > On 2017/06/02 00:58:36, rnephew (Reviews Here) wrote: > > > > > > On 2017/06/01 23:31:14, nednguyen wrote: > > > > > > > On 2017/06/01 19:14:24, rnephew (Reviews Here) wrote: > > > > > > > > > > > > > > Why do we need this? > > > > > > > > > > > > In the 'How telemetry disables tests' document I wrote and you helped > > with > > > > (or > > > > > > Charlie did? I can't recall which at the moment). Under ShouldDisable > we > > > say > > > > > > 'Maybe remove'. I was operating under the assumption we still wanted > to > > > get > > > > > rid > > > > > > of this. See > > > > > > > > > > > > > > > > > > > > > https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli... > > > > > > for an example of it being used. > > > > > > > > > > More on why we want this. The PermanentlyDisable version will not run > with > > > > > --also-run-disabled-tests since its for tests that just flat out cannot > > run > > > on > > > > > the given platform; but this one will which will make local reproduction > > > > easier > > > > > for developers for disabling that is done with the TemporarilyDisable > > > method. > > > > > > > > For TemporarilyDisable test, I expect people should just use it at story > > > level, > > > > and not at benchmark level. > > > > > > So just to confirm the preferred path forward: > > > I should scrap this CL completely, and in the other CL move the temporary > > story > > > disabling back to ShouldDisable? > > > > Most benchmarks that are currently disabled are due to the fact that we didn't > > have ways to disabling failing stories back then. I would suggest enabling > them > > all & only disable failing stories. > > > > For benchmarks that we would refactor soon like smoothness.*, power.*, you can > > just use PermanentlyDisable(..) to make the work easier. > > I think that should be done in a separate CL though, so for the current 2 cls, > I'll do my plan. Then in a follow up CL get rid of ShouldDisable, look and see > what starts failing and disable stories accordingly. That way I can make it more > clear that failures are expected in the CL name so its less chance for confusion > for the sheriffs. I see. So for https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli..., I think you can just use PermanentlyDisableBenchmark to get rid of ShoulDisable soon. So to summarize: 1) Land https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli... but use PermanentlyDisableBenchmark 2) Remove ShouldDisable in Telemetry (with annoucenement) 3) Change benchmarks with PermanentlyDisableBenchmark to use story level disabling if possible. Sorry for not pay close attention to change 521885, I should have told you feel free to just use PermanentlyDisableBenchmark.
On 2017/06/02 at 02:11:46, nednguyen wrote: > On 2017/06/02 01:55:39, rnephew (Reviews Here) wrote: > > On 2017/06/02 01:45:50, nednguyen wrote: > > > On 2017/06/02 01:37:52, rnephew (Reviews Here) wrote: > > > > On 2017/06/02 01:03:25, nednguyen wrote: > > > > > On 2017/06/02 00:59:45, rnephew (Reviews Here) wrote: > > > > > > On 2017/06/02 00:58:36, rnephew (Reviews Here) wrote: > > > > > > > On 2017/06/01 23:31:14, nednguyen wrote: > > > > > > > > On 2017/06/01 19:14:24, rnephew (Reviews Here) wrote: > > > > > > > > > > > > > > > > Why do we need this? > > > > > > > > > > > > > > In the 'How telemetry disables tests' document I wrote and you helped > > > with > > > > > (or > > > > > > > Charlie did? I can't recall which at the moment). Under ShouldDisable > > we > > > > say > > > > > > > 'Maybe remove'. I was operating under the assumption we still wanted > > to > > > > get > > > > > > rid > > > > > > > of this. See > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli... > > > > > > > for an example of it being used. > > > > > > > > > > > > More on why we want this. The PermanentlyDisable version will not run > > with > > > > > > --also-run-disabled-tests since its for tests that just flat out cannot > > > run > > > > on > > > > > > the given platform; but this one will which will make local reproduction > > > > > easier > > > > > > for developers for disabling that is done with the TemporarilyDisable > > > > method. > > > > > > > > > > For TemporarilyDisable test, I expect people should just use it at story > > > > level, > > > > > and not at benchmark level. > > > > > > > > So just to confirm the preferred path forward: > > > > I should scrap this CL completely, and in the other CL move the temporary > > > story > > > > disabling back to ShouldDisable? > > > > > > Most benchmarks that are currently disabled are due to the fact that we didn't > > > have ways to disabling failing stories back then. I would suggest enabling > > them > > > all & only disable failing stories. > > > > > > For benchmarks that we would refactor soon like smoothness.*, power.*, you can > > > just use PermanentlyDisable(..) to make the work easier. > > > > I think that should be done in a separate CL though, so for the current 2 cls, > > I'll do my plan. Then in a follow up CL get rid of ShouldDisable, look and see > > what starts failing and disable stories accordingly. That way I can make it more > > clear that failures are expected in the CL name so its less chance for confusion > > for the sheriffs. > I see. So for https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli..., I think you can just use PermanentlyDisableBenchmark to get rid of ShoulDisable soon. > > So to summarize: > 1) Land https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli... but use PermanentlyDisableBenchmark > 2) Remove ShouldDisable in Telemetry (with annoucenement) > 3) Change benchmarks with PermanentlyDisableBenchmark to use story level disabling if possible. > > Sorry for not pay close attention to change 521885, I should have told you feel free to just use PermanentlyDisableBenchmark. I keep getting confused about this. I understood that the plan was: - The new expectation system is for disabling stories/benchmarks temporarily because they are now failing; they should be fixed and re-enabled. - Disabling a story/benchmark permanently, because it *can't* run on a particular platform/browser (and probably never will) should be done elsewhere (e.g. some CanRunOnPlatform/CanRunOnBrowser methods with arbitrary code in them). Did I misunderstood something? Why is there a PermanentlyDisableBenchmark method in the expectations module?
On 2017/06/02 08:54:26, perezju wrote: > On 2017/06/02 at 02:11:46, nednguyen wrote: > > On 2017/06/02 01:55:39, rnephew (Reviews Here) wrote: > > > On 2017/06/02 01:45:50, nednguyen wrote: > > > > On 2017/06/02 01:37:52, rnephew (Reviews Here) wrote: > > > > > On 2017/06/02 01:03:25, nednguyen wrote: > > > > > > On 2017/06/02 00:59:45, rnephew (Reviews Here) wrote: > > > > > > > On 2017/06/02 00:58:36, rnephew (Reviews Here) wrote: > > > > > > > > On 2017/06/01 23:31:14, nednguyen wrote: > > > > > > > > > On 2017/06/01 19:14:24, rnephew (Reviews Here) wrote: > > > > > > > > > > > > > > > > > > Why do we need this? > > > > > > > > > > > > > > > > In the 'How telemetry disables tests' document I wrote and you > helped > > > > with > > > > > > (or > > > > > > > > Charlie did? I can't recall which at the moment). Under > ShouldDisable > > > we > > > > > say > > > > > > > > 'Maybe remove'. I was operating under the assumption we still > wanted > > > to > > > > > get > > > > > > > rid > > > > > > > > of this. See > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli... > > > > > > > > for an example of it being used. > > > > > > > > > > > > > > More on why we want this. The PermanentlyDisable version will not > run > > > with > > > > > > > --also-run-disabled-tests since its for tests that just flat out > cannot > > > > run > > > > > on > > > > > > > the given platform; but this one will which will make local > reproduction > > > > > > easier > > > > > > > for developers for disabling that is done with the > TemporarilyDisable > > > > > method. > > > > > > > > > > > > For TemporarilyDisable test, I expect people should just use it at > story > > > > > level, > > > > > > and not at benchmark level. > > > > > > > > > > So just to confirm the preferred path forward: > > > > > I should scrap this CL completely, and in the other CL move the > temporary > > > > story > > > > > disabling back to ShouldDisable? > > > > > > > > Most benchmarks that are currently disabled are due to the fact that we > didn't > > > > have ways to disabling failing stories back then. I would suggest enabling > > > them > > > > all & only disable failing stories. > > > > > > > > For benchmarks that we would refactor soon like smoothness.*, power.*, you > can > > > > just use PermanentlyDisable(..) to make the work easier. > > > > > > I think that should be done in a separate CL though, so for the current 2 > cls, > > > I'll do my plan. Then in a follow up CL get rid of ShouldDisable, look and > see > > > what starts failing and disable stories accordingly. That way I can make it > more > > > clear that failures are expected in the CL name so its less chance for > confusion > > > for the sheriffs. > > I see. So for > https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli..., > I think you can just use PermanentlyDisableBenchmark to get rid of ShoulDisable > soon. > > > > So to summarize: > > 1) Land > https://chromium-review.googlesource.com/c/521885/5/tools/perf/benchmarks/bli... > but use PermanentlyDisableBenchmark > > 2) Remove ShouldDisable in Telemetry (with annoucenement) > > 3) Change benchmarks with PermanentlyDisableBenchmark to use story level > disabling if possible. > > > > Sorry for not pay close attention to change 521885, I should have told you > feel free to just use PermanentlyDisableBenchmark. > > I keep getting confused about this. I understood that the plan was: > > - The new expectation system is for disabling stories/benchmarks temporarily > because they are now failing; they should be fixed and re-enabled. > > - Disabling a story/benchmark permanently, because it *can't* run on a > particular platform/browser (and probably never will) should be done > elsewhere (e.g. some CanRunOnPlatform/CanRunOnBrowser methods > with arbitrary code in them). > > Did I misunderstood something? Why is there a PermanentlyDisableBenchmark method > in the expectations module? I will do my best to explain the slight change of direction here. 1) The new system is mostly about disabling failing stories. 90% of benchmark disabling are due to that, and they are not permanent disable. 2) In the new world, assuming we kill all the old disabling ways, then the only cases people want to disable the whole benchmark is disabling permanently. Example: disabling a benchmark if there is no battor, disabling a benchmark on dekstop or mobile. 3) Randy wants to have a single area of code for the disabling logic, so he put the PermanentlyDisableBenchmark in the expectations module. <-- This is the controversial part, but I don't have strong feeling about this.
On 2017/06/02 at 10:39:22, nednguyen wrote: > 3) Randy wants to have a single area of code for the disabling logic, so he put the PermanentlyDisableBenchmark in the expectations module. <-- This is the controversial part, but I don't have strong feeling about this. Yeah, I would prefer to keep the two systems separate. Expectations sound find for more ephemeral code, where stories are constantly being added/removed as needed. And there it's also bearable, I think, to have some code repetition if we want to disable the same story on different benchmarks. On the other hand, for permanent "disables" I would very much prefer to have some say "DesktopBenchmark" which I can subclass to have something that only ever runs on desktop platforms. Rather than explicitly having to say ".. and disable this permanently on android" on all of them.
On 2017/06/02 11:16:16, perezju wrote: > On 2017/06/02 at 10:39:22, nednguyen wrote: > > 3) Randy wants to have a single area of code for the disabling logic, so he > put the PermanentlyDisableBenchmark in the expectations module. <-- This is the > controversial part, but I don't have strong feeling about this. > > Yeah, I would prefer to keep the two systems separate. There are only 3 cases of permanently benchmark disable I know so far: mobile vs dekstop, battor is available. I also expect that the number of cases there won't grow, so I am fine with either way. Maybe we should just keep the Benchmark.ShouldDisable method (or rename it to Benchmark.ShouldDisablePermanently) as this make a great counterpart to Page.CanRunOnBrowser stuff. > > Expectations sound find for more ephemeral code, where stories are > constantly being added/removed as needed. And there it's also > bearable, I think, to have some code repetition if we want to > disable the same story on different benchmarks. > > On the other hand, for permanent "disables" I would very much > prefer to have some say "DesktopBenchmark" which I can subclass > to have something that only ever runs on desktop platforms. > Rather than explicitly having to say ".. and disable this > permanently on android" on all of them. I agree with the sentiment here. Having a DesktopBenchmark & MobileBenchmark subclasses in the base telemetry.benchmark module sgtm.
On 2017/06/02 at 11:41:04, nednguyen wrote: > On 2017/06/02 11:16:16, perezju wrote: > > On 2017/06/02 at 10:39:22, nednguyen wrote: > > > 3) Randy wants to have a single area of code for the disabling logic, so he > > put the PermanentlyDisableBenchmark in the expectations module. <-- This is the > > controversial part, but I don't have strong feeling about this. > > > > Yeah, I would prefer to keep the two systems separate. > > There are only 3 cases of permanently benchmark disable I know so far: mobile vs dekstop, battor is available. I also expect that the number of cases there won't grow, so I am fine with either way. Maybe we should just keep the Benchmark.ShouldDisable method (or rename it to Benchmark.ShouldDisablePermanently) as this make a great counterpart to Page.CanRunOnBrowser stuff. Agreed. We should rename it to Benchmark.CanRunOnBrowser/CanRunOnPlatform; which also avoids some awkward double negatives like "disable if not battor" when what you actually mean is "needs battor to run". > > > > Expectations sound find for more ephemeral code, where stories are > > constantly being added/removed as needed. And there it's also > > bearable, I think, to have some code repetition if we want to > > disable the same story on different benchmarks. > > > > On the other hand, for permanent "disables" I would very much > > prefer to have some say "DesktopBenchmark" which I can subclass > > to have something that only ever runs on desktop platforms. > > Rather than explicitly having to say ".. and disable this > > permanently on android" on all of them. > > I agree with the sentiment here. Having a DesktopBenchmark & MobileBenchmark subclasses in the base telemetry.benchmark module sgtm.
Description was changed from ========== [Telemetry] Add temporary disabling of benchmark to story expectations. Before we only supported permanent disabling. This was so that tests that we know cant run on certain platforms (tests that use android specific calls) are not accidently run on other platforms. This adds the ability to disable failing benchmarks on a temporary basis. BUG=chromium:713222 ========== to ========== [Telemetry] Add temporary disabling of benchmark to story expectations. Before we only supported permanent disabling. This was so that tests that we know cant run on certain platforms (tests that use android specific calls) are not accidently run on other platforms. This adds the ability to disable failing benchmarks on a temporary basis. BUG=chromium:713222 ==========
Message was sent while issue was closed.
On 2017/06/02 11:51:37, perezju wrote: > On 2017/06/02 at 11:41:04, nednguyen wrote: > > On 2017/06/02 11:16:16, perezju wrote: > > > On 2017/06/02 at 10:39:22, nednguyen wrote: > > > > 3) Randy wants to have a single area of code for the disabling logic, so > he > > > put the PermanentlyDisableBenchmark in the expectations module. <-- This is > the > > > controversial part, but I don't have strong feeling about this. > > > > > > Yeah, I would prefer to keep the two systems separate. > > > > There are only 3 cases of permanently benchmark disable I know so far: mobile > vs dekstop, battor is available. I also expect that the number of cases there > won't grow, so I am fine with either way. Maybe we should just keep the > Benchmark.ShouldDisable method (or rename it to > Benchmark.ShouldDisablePermanently) as this make a great counterpart to > Page.CanRunOnBrowser stuff. > > Agreed. We should rename it to Benchmark.CanRunOnBrowser/CanRunOnPlatform; which > also avoids some awkward double negatives like "disable if not battor" when what > you actually mean is "needs battor to run". > > > > > > > Expectations sound find for more ephemeral code, where stories are > > > constantly being added/removed as needed. And there it's also > > > bearable, I think, to have some code repetition if we want to > > > disable the same story on different benchmarks. > > > > > > On the other hand, for permanent "disables" I would very much > > > prefer to have some say "DesktopBenchmark" which I can subclass > > > to have something that only ever runs on desktop platforms. > > > Rather than explicitly having to say ".. and disable this > > > permanently on android" on all of them. > > > > I agree with the sentiment here. Having a DesktopBenchmark & MobileBenchmark > subclasses in the base telemetry.benchmark module sgtm. I'm going ahead and closing this review out since we are not moving on with the changes in it. I will start the work on removing ShouldDisable, and will move the other CL to use PermDisableBenchmark for now. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
