Skip to content

Stop using fake noteable_path and object_kind if possible

Lin Jen-Shin requested to merge simplify-stubbed-event into master

What does this MR do and why?

This is a follow up from !2380 (merged) and part of #968 to avoid this from happening ever again: #1376 (closed)

The around half year long incident was fixed by !2375 (merged) where we added the missing implementation noteable_kind for IncidentEvent. Without it, the application raises NotImplementedError and halt all the processors, potentially crashing the application.

Why was it not detected by the tests? Because we're faking too many things in the fake events. We stubbed noteable_path which will use noteable_kind. This merge request removes the following stubs:

  • object_kind -- default removed, TODO: remove all the stubs for all tests. It doesn't make sense to stub it unless we want something abnormal. This should be completely based on the event we're using.
  • key -- I don't think this is used?
  • noteable_path -- Now we can detect NotImplementedError better
  • url -- I don't think this is used.
  • payload -- the default implementation is exactly returning it, and there's no point to stub it again.

The rationale going down the path to greatly remove stubs was described in the description of !2380 (merged)

Expected impact & dry-runs

These are strongly recommended to assist reviewers and reduce the time to merge your change.

See https://gitlab.com/gitlab-org/quality/triage-ops/-/tree/master/doc/scheduled#testing-policies-with-a-dry-run on how to perform dry-runs for new policies.

See https://gitlab.com/gitlab-org/quality/triage-ops/-/blob/master/doc/reactive/best_practices.md#use-the-sandbox-to-test-new-processors on how to make sure a new processor can be tested.

Action items

Part of #968

Edited by Lin Jen-Shin

Merge request reports