search

dart-lang / dart_ci

Tools used by Dart's continuous integration (CI) testing that aren't needed by Dart SDK contributors. Mirrored from dart.googlesource.com/dart_ci. Do not land pull requests on Github.
BSD 3-Clause "New" or "Revised" License
18 stars 5 forks source link

detecting stale approvals #138

Open mraleph opened 1 year ago

mraleph commented 1 year ago

We have accidentally discovered that one of the language tests essentially had a stale approval since approximately February. It was marked as "expected CompileTimeError" during the development of the language feature, but this approval was never cleared even though we have considered that we finished implementing the feature.

It would be nice if we had a mechanism in place to prevent this from happening.

One option I can see here is that approving a failing test should always require a GitHub issue link, which would allow us to periodically check if corresponding issue is closed and then clear the approval.

Maybe there are other options here. e.g. we probably should monitor approval database for status discrepancies between analyzer and cfe-* configurations and report them (e.g. if analyzer test passes but CFE reports CompileTimeError this is suspicious).

/cc @athomas @whesse @leafpetersen @chloestefantsova

leafpetersen commented 1 year ago

One option I can see here is that approving a failing test should always require a GitHub issue link

I think this would be ok assuming we don't need a separate issue per test. The case I have in mind here is that I write a whole bunch of tests for a new language feature (which immediately start failing), and then I land them with approved failures. I'd be fine linking the approval to the meta-issue for the feature if that helped, but not with writing a separate issue for every test. We'd have to have a process in place for doing the same on a co19 roll.

I think for language features specifically, the goal was that we would track this in the burn down test tracking sheets, and when we arrived at launch time, there would be a step where someone verified that all tests listed in the tracking sheet were passing. I'm guessing that I dropped the ball on that with the enhanced enums feature. Hopefully now that the actual professionals are on the job (cc @itsjustkevin ) we can get better about this.

At a meta-level, we could really stand to do an audit of our failing tests. That's probably on the language team. :)