Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate Random Rather than Sequential Test Emails #51400

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Apr 19, 2023

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
Copy link
Member

@davidsbailey davidsbailey left a comment

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
Copy link
Contributor Author

Hamms commented Apr 20, 2023

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
2 checks passed
@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.

None yet

2 participants