refactor the flutter-analyze try bot and downstream failure notifications

devoncarew commented 3 years ago

This is a meta issue to track work related to 1) adding more repositories to our flutter-analyze try bot, and 2) relax some of the analysis settings on the flutter/tests contributed tests repo. The general goal for these changes are to enable the analyzer team to be aware of breakages that their changes would cause, know which places will need to be forward patched to not impede the sdk -> flutter roll process, and to omit a class of issues from needing to be patched (to not break the roll process for some category of changes).

Currently most (all?) of the flutter/tests user contributed tests fail on any analysis change. However, the intent here is more to make sure that flutter is aware of breaking API changes. Below, I propose to change the tests to only fail on more severe analysis issues (errors or warnings). We may in the future make this more sophisticated - to leverage package:flutter_lints in some capacity to also fail on some set of lint warnings.

[x] have flutter-analyze analyze flutter/flutter (it's doing this today; the potential improvement to this being to also run analysis for dartdoc sample code - https://github.com/flutter/flutter/blob/master/dev/bots/analyze_sample_code.dart)
[x] have flutter-analyze analyze flutter/plugins; see https://github.com/dart-lang/sdk/issues/45873
[ ] have flutter-analyze analyze flutter/engine; we're doing this now, but might want to do some additional work to make it easier to use the same analysis script as what flutter/engine does
[ ] have flutter-analyze analyze flutter/packages
[x] update user contributed scripts in the flutter/tests registry/ dir to only fail on errors (or possibly warnings); see https://github.com/flutter/flutter/pull/82778/checks?check_run_id=2606817706 for a list of failed locations for the recent unused imports change (PR: https://github.com/flutter/tests/pull/105)
[x] update the readme in flutter/tests to indicate that contributed tests should not fail on info level analysis items (or, possibly, warning level items)
[ ] come back and decide whether we want to change the flutter/tests analysis criteria to be more sophisticated than just being based on analysis issue severity (perhaps using something based on package:flutter_lints?)
[ ] decide whether we want to exclude some flutter repos from our try bot (flutter/cocoon ?) and rely on the HHH bot to catch issues post-commit

See https://github.com/dart-lang/sdk/issues/46063 for more context; https://github.com/flutter/tests/tree/master/registry for the flutter customer tests repo.

devoncarew commented 3 years ago

cc @Hixie @goderbauer - if we can agree on some of the items above (wrt flutter/tests changes), I'll follow up with some PRs to flutter/tests

goderbauer commented 3 years ago

Is there an overview of what the analyzer considers an error or warning? I am curious to see what issues we would still catch for packages who have contributed tests to the tests repository.

devoncarew commented 3 years ago

Most actual issues related to an API refactoring would be error level severity issues. Lints and hints (essentially built-in lints) are info level severity. There are actually very few warning level severity items (there were more in the Dart 1 days).

Lints can't emit warning level items, however, an analysis options file can later upgrade them to be warning level items for a project. I'll poke around for a list of the warning level items.

devoncarew commented 3 years ago

Adding the full set of analysis infos, warnings, and errors.

infos.txt warnings.txt errors.txt

And here's the set of warnings, inlined:

analysis_option_deprecated
camera_permissions_incompatible
dead_null_aware_expression
include_file_not_found
included_file_warning
invalid_dependency
invalid_null_aware_operator
invalid_null_aware_operator
invalid_option
invalid_override_different_default_values_named
invalid_override_different_default_values_positional
invalid_section_format
missing_enum_constant_in_switch
no_touchscreen_feature
non_resizable_activity
path_does_not_exist
path_pubspec_does_not_exist
permission_implies_unsupported_hardware
setting_orientation_on_activity
spec_mode_removed
unnecessary_non_null_assertion
unrecognized_error_code
unsupported_chrome_os_feature
unsupported_chrome_os_hardware
unsupported_option_with_legal_value
unsupported_option_with_legal_values
unsupported_option_without_values
unsupported_value

goderbauer commented 3 years ago

I couldn't find any documentation on these warnings. Does the presence of these may mean that your app doesn't compile/run anymore?

In other words, I am fine with this proposal as long as we still get notified if we break packages in the tests repository in the sense that their code doesn't compile or run anymore.

devoncarew commented 3 years ago

I removed some of the warnings for pubspec files and other non-dart files, and added in the descriptions:

dead_null_aware_expression:
  https://dart.dev/tools/diagnostic-messages#dead_null_aware_expression
  The left operand can't be null, so the right operand is never executed.

invalid_null_aware_operator:
  https://dart.dev/tools/diagnostic-messages#invalid_null_aware_operator
  The receiver can't be null, so the null-aware operator '{0}' is unnecessary.

invalid_null_aware_operator:
  https://dart.dev/tools/diagnostic-messages#invalid_null_aware_operator
  The receiver can't be null because of short-circuiting, so the null-aware operator '{0}' can't be used.

invalid_override_different_default_values_named:
  Parameters can't override default values, this method overrides '{0}.{1}' where '{2}' has a different value.

invalid_override_different_default_values_positional:
  Parameters can't override default values, this method overrides '{0}.{1}' where this positional parameter has a different value.

missing_enum_constant_in_switch:
  https://dart.dev/tools/diagnostic-messages#missing_enum_constant_in_switch
  Missing case clause for '{0}'.

unnecessary_non_null_assertion:
  https://dart.dev/tools/diagnostic-messages#unnecessary_non_null_assertion
  The '!' will have no effect because the receiver can't be null.

devoncarew commented 3 years ago

In other words, I am fine with this proposal as long as we still get notified if we break packages in the tests repository in the sense that their code doesn't compile or run anymore.

Yes, that'll be the case - for those kinds of breakages, we'll emit error level issues.

goderbauer commented 3 years ago

Looking at that list, it would make sense to fail on errors and warnings. For example, if we add a new value to an existing enum (which presumably triggers missing_enum_constant_in_switch) we'd want that to be treated as a breaking change.

devoncarew commented 3 years ago

If it's just that one code - missing_enum_constant_in_switch - I think I'd like to look to make this more restrictive in the future; to look to how we could switch to just using errors (promoting that warning to an error)?

But I think we can tackle that as part of the 'come back and decide whether we want to change the flutter/tests analysis criteria' checkbox above. For now, just ignoring info level items sgtm.

devoncarew commented 3 years ago

A PR to flutter/tests to update the static analysis and the test guidance: https://github.com/flutter/tests/pull/105. We may evolve the criteria we check on in the future (ignore warnings? use errors and a sub-set of package:flutter_lints?).

devoncarew commented 3 years ago

https://github.com/dart-lang/sdk/issues/46075 will update the flutter-analyze bot to also check the dartdoc samples.

dart-lang / sdk

refactor the flutter-analyze try bot and downstream failure notifications #46075