Skip to content

Generate Random Rather than Sequential Test Emails - #51400

Merged
Hamms merged 1 commit into
stagingfrom
generate-random-test-emails
Apr 20, 2023
Merged

Generate Random Rather than Sequential Test Emails#51400
Hamms merged 1 commit into
stagingfrom
generate-random-test-emails

Conversation

@Hamms

@Hamms Hamms commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

We expect emails to be globally unique, but the method we currently use to generate test emails for facilitators and workshop organizers means that multiple calls to build :workshop_organizer will result in the same email being generated for each user, which will cause an error if we attempt to persist both of them.

This isn't a problem right now, because some unexpected behavior in FactoryBot v4 means that records are often automatically persisted upon creation. Starting in v5, we are much more likely to encounter the edge case of multiple records being initially built but not persisted only for a subset of them to be persisted after the fact. In anticipation of updating to that version, we change the generation logic to use unique emails rather than attempting to enforce an arbitrary sequence.

Links

I did some digging into the history of this email generation logic, and couldn't find any evidence that we cared about the sequential nature of the old approach; it seemed like we were just going with the simplest, cleanest option in the absence of a need for anything this complicated.

Testing story

Verified on another branch that this change resolves the issues with FactoryBot v5.

Follow-up work

Upgrade FactoryBot to v5

We expect emails to be globally unique, but the method we currently use to generate test emails for facilitators and workshop organizers means that multiple calls to `build :workshop_organizer` will result in the same email being generated for each user, which will cause an error if we attempt to persist both of them.

This isn't a problem right now, because some unexpected behavior in FactoryBot v4 means that record are often automatically persisted upon creation. Starting in v5, we are much more likely to encounter this edge case, so in anticipation of updating to that version we change the generation logic to use unique emails rather than attempting to enforce an arbitrary sequence.
@Hamms
Hamms marked this pull request as ready for review April 19, 2023 21:58
@Hamms
Hamms requested a review from a team April 19, 2023 22:00
@Hamms Hamms added the Ruby Update Everything related to work to update the version of Ruby our codebase runs on label Apr 19, 2023

@davidsbailey davidsbailey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the thoughtful analysis, @Hamms ! Based on the information you've laid out, I think the solution you have here is simplest and best. Just to confirm my understanding, switching from build to create would also work, but is undesirable because it creates extra DB queries, is that correct?

@Hamms

Hamms commented Apr 20, 2023

Copy link
Copy Markdown
Contributor Author

Yep! Although I'm not particularly worried about extra records, since they get cleaned up at the end of a test run anyway. It's mostly undesirable because in order to switch from build to create everywhere, we'd have to track down anywhere that any other factory is trying to build one of these as an associated record, and also make sure that any future factories don't try to do that either. This approach is more holistic

@Hamms
Hamms merged commit 66e7d8c into staging Apr 20, 2023
@Hamms
Hamms deleted the generate-random-test-emails branch April 20, 2023 20:31
@Hamms Hamms mentioned this pull request Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ruby Update Everything related to work to update the version of Ruby our codebase runs on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants