☂️ Skip test audit 2021 #86396

[Updated from a previous approach. I explain the reason for the at the bottom for context.]

In order to reduce our technical debt, we are performing an audit of all tests that are marked with skip to ensure that we have a few of them as possible. Currently these skipped tests produce about half of our technical debt calculation (1080701 of 2346215 as of 6/30/21).

Tests are skipped for two main reasons. The first is to temporarily shut off a test that is causing tree breakage. That allows the tree to stay open while a fix is developed. For this case we are going to require a comment on the same line as the skip parameter with a URL to a github issue to track these tests and make sure they are eventually fixed. These issues should be tagged with the skip-test label to make them easier to find.

The other main reason for test skipping is because the test just isn't designed to work under some circumstances. For example, when testing for page transitions, we have no default transitions on the web so it doesn't make sense to test them on the web platform. For these tests we are going to require a comment on the same line as the skip parameter with the text [intended] preferably followed by a description as to why it is intended. This will greatly speed up audits in the future as we won't need to sort out which tests were skipped intentionally and which were just a temporary way to keep the tree green.

Audit

For each skipped test we will need to perform the following:

• Verify the test still needs to be skipped. If it doesn't, remove the skip parameter.

• If the test should always be skipped under the given condition, add a trailing comment with [intend] followed by the reason it is an intentional skip. For example:

    }, skip: !platform.isMacOS); // [intended] codesign is only available on macOS

• Otherwise, the skip should be temporary, so ensure there is an issue filed for it with a label of skip-test and a link to the issue is in a trailing comment. For example:

    }, skip: true); // https://github.com/flutter/flutter/issues/87285

Tasks

• Add an exclude boolean parameter to testWidgets. Added 'exclude' parameter to 'testWidgets()'. #86397
• Revert the above commit that added the exclude parameter Revert "Added 'exclude' parameter to 'testWidgets()'." #87515
• Update the Tree hygiene page on the wiki with a section explaining skip test handling.
• Add a check to the analyzer script to detect a skip test without a following comment with [intended] or a tracking issue. Added a check to the analyzer script to detect skipped tests. #88003

Audit groups

• ./dev (16 tests) Update the skipped test for dev. #87147
• ./packages/flutter/test/animation (6 tests) Updated the skipped tests for animation #87063
• ./packages/flutter/test/cupertino (22 tests) Updated the skipped tests for cupertino package. (reland) #87538
• ./packages/flutter/test/foundation (11 tests) Updated skipped tests for foundation directory. #87293
• ./packages/flutter/test/gestures (1 test) Update the skipped test for gestures. #87130
• ./packages/flutter/test/material (94 tests) Updated skipped tests for material directory. #87328
• ./packages/flutter/test/painting (55 tests) Updated skipped tests for painting directory. #87546
• ./packages/flutter/test/rendering (88 tests) Updated skipped tests for rendering directory. #87700
• ./packages/flutter/test/scheduler (1 test) Updated skipped tests for scheduler directory. #87873
• ./packages/flutter/test/services (33 tests) Updated skipped tests for services directory. #87874
• ./packages/flutter/test/widgets (82 tests) Updated skipped tests for widgets directory. #87879
• ./packages/flutter_test/test (3 tests) Updated skipped tests for flutter_test directory. #87880
• ./packages/flutter_tools/test (28 tests) Updated skipped tests for flutter_tools. #87925

Previous approach

The previous approach was to introduce a new 'exclude' parameter to testWidgets. However, this got some push back and wound up being insufficient. There are a lot of tests in the repo only using test or group from the dart test package, so it would involve a much bigger change to add the parameter to those as well. Instead, I opted for the simpler method of just enforcing a specific comment tag on intended skips. Here was the original plan:

The intent of skip is to temporarily shut off a failing test so that the tree can be kept open while a fix is worked on. However, there are many tests in our tree that are using skip to indicate a test that should never be run under some condition, as it just isn't designed for it.

To be able to tell the difference between a test that is temporarily shut off and one not designed to work for a certain condition, we are going to introduce a new boolean exclude parameter to testWidgets. This will work the same as skip, but will be used to indicate the skip is by design and needs to further attention._