New issue
Advanced search Search tips

Issue 830341 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Feature Request: disable unit tests/browser tests on dashboard

Project Member Reported by shimazu@google.com, Apr 9 2018

Issue description

I'm not quite sure if this FR is good to be tagged as SheriffOMatic, but I feel it's awkward to cut a local branch, add DISABLED_ prefix to some tests or add MAYBE_ and some #if defined(...) macros, and find proper reviewers for TBR-ing etc.

I assume that it's possible to be somewhat automated. We can avoid some typical mistakes like forgetting to add MAYBE_ prefix though "#define MAYBE_Sometest" is added.
It'll make the tasks for (at least) build sheriff much smoother.

My proposal is to have some UI to:
- Fill the name of tests to disable/enable in a box
- Select some conditions if needed (ADDRESS_SANITIZER, OS_LINUX etc)
- Select trybots if needed
- Select reviewers (and auto-populated if possible)

and it generates a CL to submit.

Do you think it's feasible?
 
Cc: dpranke@chromium.org
If you write your tests to use Test Expectations config files*, SoM can indeed help you with this.

For instance, webkit layout tests use these files and SoM can edit them automatically: 
https://sheriff-o-matic-staging.appspot.com/test-expectations

We specifically limited the scope of this feature to tests that use expectations files. If you need to edit source code to enable/disable tests, that's out of scope for SoM.

*See https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_test_expectations.md for more information.
bit.ly/chromium-test-list-format is also a good link to be aware of.

However, there has been a lot of pushback from other devs against adding text expectation files for gtest-based tests, given that you can use DISABLED_ and MAYBE_, so we haven't added this feature before. Once we have enough experience with the SoM-based approach, we need to revisit that decision.

@seanmccullough - we don't support SoM-based disabling for telemetry-based tests yet, right? IIRC that's waiting on me to implement the support in TYP.
Re #2, that is my understanding. 

We also need to convert the existing expectations files to bit.ly/chromium-test-list-format IIRC.
> We also need to convert the existing expectations files to
> bit.ly/chromium-test-list-format IIRC.

Good point (and also on my plate).

Comment 5 by mmoss@chromium.org, Apr 10 2018

Components: -Infra
Components: -Infra>Sheriffing>SheriffOMatic
This isn't a Sheriff-o-Matic issue.
@seanmccullough - isn't this requesting TADA for gtest-based tests?
re #7 Yes that's their desired outcome, but gtests (AFAIK) aren't set up to be compatible with TA/DA. We specifically rejected plans to have TADA edit C++ (or any other) source code files. 

They'll need to modify their tests to use expectations files before TADA can help them. That sounds like a big project that's way outside of SoM's scope. 

That said, we could leave this under the SoM component if there's no where else to put it. In that case though, it should probably be blocked on a separate "change all your tests to use expectations files instead of macros" bug assigned to them. SoM can't make any progress on this as-is.

I agree with your assessment; I think the thing to do is file a bug for the gtest work and block this work on that.
Components: Infra>Sheriffing>SheriffOMatic
Status: Available (was: Untriaged)
Labels: Milestone-Workflow

Sign in to add a comment