Use Builder for InflightActivation

[james-mcnulty]

Description

Right now, every InflightActivation in the unit tests is constructed manually field by field. This is extremely verbose and largely unnecessary, as most fields are set to use default values (for example, 0 when the type is an integer or None when the type is an Option).

We can reduce this boilerplate by using the builder pattern to construct these structs instead. That way, you only need to set the fields you are interested in, and it's easier to tell what's different between two InflightActivations as you don't need to parse a million irrelevant fields that are all the same.

For a demonstration, consider the two activations A and B below. What's the difference between them?

Activation A

InflightActivation {
    id: "task-123".to_string(),
    namespace: "my-namespace".to_string(),
    taskname: "my-task".to_string(),
    activation: TaskActivation {
        id: "task-123".to_string(),
        namespace: "my-namespace".to_string(),
        taskname: "my-task".to_string(),
        parameters: "{}".to_string(),
        headers: HashMap::new(),
        processing_deadline_duration: 0,
        received_at: None,
        retry_state: None,
        expires: None,
        delay: None,
    }.encode_to_vec(),
    status: InflightActivationStatus::Pending,
    partition: 0,
    offset: 0,
    added_at: Utc::now(),
    received_at: Utc::now(),
    processing_attempts: 0,
    processing_deadline_duration: 0,
    expires_at: None,
    delay_until: None,
    processing_deadline: None,
    on_attempts_exceeded: OnAttemptsExceeded::Discard,
    at_most_once: false
}

Activation B

InflightActivation {
    id: "task-123".to_string(),
    namespace: "my-namespace".to_string(),
    taskname: "my-task".to_string(),
    activation: TaskActivation {
        id: "task-123".to_string(),
        namespace: "my-namespace".to_string(),
        taskname: "my-task".to_string(),
        parameters: "{}".to_string(),
        headers: HashMap::new(),
        processing_deadline_duration: 0,
        received_at: None,
        retry_state: None,
        expires: None,
        delay: None,
    }.encode_to_vec(),
    status: InflightActivationStatus::Pending,
    partition: 0,
    offset: 10,
    added_at: Utc::now(),
    received_at: Utc::now(),
    processing_attempts: 0,
    processing_deadline_duration: 0,
    expires_at: None,
    delay_until: None,
    processing_deadline: None,
    on_attempts_exceeded: OnAttemptsExceeded::Discard,
    at_most_once: false
}

Activation A has an offset of 0 while B has an offset of 10. Even if it didn't take very long to figure that out, you still had to read quite a few irrelevant fields that are the same between both activations.

Now consider the same two activations constructed using a builder. We don't need to read all the fields because only the offset setter is used! This tells us almost immediately that the offset is the only value that differs between the two tasks.

Activation A

InflightActivationBuilder::default()
    .id("task-123")
    .namespace("my-namespace")
    .taskname("my-task")
    .offset(0)
    .activation(
        TaskActivationBuilder::default()
            .id(id)
            .namespace("my-namespace")
            .taskname("my-task")
            .build()
    )
    .build()

Activation B

InflightActivationBuilder::default()
    .id("task-123")
    .namespace("my-namespace")
    .taskname("my-task")
    .offset(10)
    .activation(
        TaskActivationBuilder::default()
            .id(id)
            .namespace("my-namespace")
            .taskname("my-task")
            .build()
    )
    .build()

As a result, it should be easier to write, read, and understand tests.

Details

• Used the derive_builder crate to generate a builder for InflightActivation with reasonable defaults
• Manually implemented a builder for TaskActivation since it's generated by sentry-protos
• Replaced every occurance of InflightActivation { ... } in the tests with the builder

[james-mcnulty]

Since the new builder boilerplate takes around 150 lines, these changes remove nearly 300 lines from the tests overall. We could cut even more code by...

• Generating the builder for TaskActivation in sentry-protos instead
• Building the TaskActivation inside of the InflightActivationBuilder instead of requiring the user to provide an activation

[markstory]

Nice work. This should make writing tests simpler.

[markstory]

It would be nice if we could infer the activation properties from that are shared with the InflightActivation when using these builders. That could eliminate more boilerplate from tests and prevent mistakes where property values can diverge.

[james-mcnulty]

Good point, I'll try this out!

[james-mcnulty]

Done, it's even better now. In most tests, you can entirely avoid setting any values for the inner TaskActivation. And I actually found some mistakes where property values diverge. For example, expires was set on the TaskActivation but not the InflightActivation in several tests.

[markstory]

Should this be implemented?

[james-mcnulty]

I was thinking about writing some tests to make sure my builders are good, but I changed my mind because there are way too many combinations to check. I figure if the unit tests still pass, my builders work fine.