Skip to content

GitLab EventStore: Is the Event definition test necessary?

Context

I've been adding quite a few events (&7920 (closed)) using the GitLab EventStore (https://docs.gitlab.com/ee/development/event_store.html).

For the events that I added I followed the documentation and examples we already had in the codebase. Therefore, as you can see in this example !92690 (merged), for each event I added:

  1. Event definition
  2. Code to publish the event
  3. Test for the event definition
  4. Test for the event being published

This issue is to discuss if the 3. Test for the event definition is really necessary.

How the test is done

As an example: https://gitlab.com/gitlab-org/gitlab/-/blob/master/spec/events/projects/project_path_changed_event_spec.rb

For each event definition we use one (or a few) valid events and a few of invalid events to test if this list of events matches with the valid/invalid status.

To validate an event we test it against its schema, if it raises Gitlab::EventStore::InvalidEvent, the event is invalid, otherwise it's valid.

Why I think we don't need these tests

(pros) The advantage, as far as I can see, to adding these tests is to have a list of examples of events. But since the schemas already describes the events, I think it's quite straightforward to envision valid and invalid events.

(cons) I don't think this aggregates value because:

  • As far as I understand, these tests are validating that the JSON scheme library is working as expected, I think we should trust it works;
  • These tests are getting more and more complex, by trying to add more examples of invalid jsons (example of conversation around that: !92690 (comment 1046808685));

Proposal

Remove these specs to win a little time on the CI and reduce complexity in MR code reviews.