-
Notifications
You must be signed in to change notification settings - Fork 64
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
[DRAFT] New technique for test data factories #403
Conversation
Thanks for opening this up! Before I give any feedback I'd like to learn what problems you'd like to solve with Boxes and how this solves them better. I don't mean this to say Boxes are better, I'm just trying to understand. My gut feeling though is that I'd rather add a couple methods to Boxes as they are, but maybe I'm misunderstanding the use-cases and the other proposals will not solve them: #385 and #331 |
Yes, so the main things are:
I might think of more details later. This design was not my original layout. The original looked like: class UserFactory < Avram::Factory
name "foo"
email "foo@example.com"
end but I ran into issues with this design when trying to add inheritance like is used in the specs currently. I switched to this module macro method setup to avoid that issue and remove inheritance as an option (I guess it might still be possible since it makes a class but I didn't want to solve or consider that). All of these things can be solved by Boxes as well (with a mess of macros like this of course) and that was my original intention but I made a separate class for clarity. The generated class is not all that different from boxes right now so it definitely is not a 180 degree departure. I can make a PR with this as a Box update later 👍 |
@matthewmcgarvey Ah ok I get what you're saying. I think we are on the same page in terms of goals. The reason I went with raw classes with minimal macros or DSL was this: using a Crystal class/methods allows you to do most of the fancy stuff that FactoryBot does, but without needing a DSL or extra API. For example, if you want a trait, it'd just be a method: class RepoBox
def open_source
contributing_file "some contributing text"
homepage "example.com"
public true
end
def with_tag(tag_name : String)
after_save do |repo|
TagBox.create &.tag_name(tag_name).for(repo)
end
end
end
class TagBox
def for(repo : Repo)
after_save do |tag|
TaggingBox.create &.tag_id(tag.id).repo_id(repo.id)
end
end
end
RepoBox.create &.open_source.with_tag("ruby") The cool part about this is that you can also pass values to traits as arguments, instead of needing "transient" attributes. Transient attributes are always a bit confusing because they are often disconnected from where they are used. Using a regular method means you can pass those attributes as an argument. To make this work though we'd need #331. And #385 would solve the issues around associations, which I agree are an issue right now! I think I'd prefer adding features for those two issues so that we can get all the "traits/transient attributes/etc." for free. I think another thing is we need to just document these patterns so people are away of how you might use methods as "traits" and as "transient" attributes. |
@paulcsmith I worked around the association issues by using the https://github.com/luckyframework/avram/pull/403/files#diff-a301750de90653814ccc43126b0883f2R42 |
@matthewmcgarvey Oh I think that is fine to use the model 👍 I thought they were also on SaveOperation, but this makes sense |
We've talked quite a bit about the rough edges of Boxes. This is a rough sketch of an alternative I have in mind. It is super macro heavy but usage is very simple.
Box Example
Factory Example
(Both assume the user box/factory is already made)
It can handle traits, property overrides, and returns the model with associations attached if they were made.
The api was heavily inspired by factory_bot.
NOTE: This is not production ready code. It is only meant for discussion purposes.
NOTE 2: I wanted to make this a separate library but, wow, this codebase has an involved test setup and I didn't want to spend ~30 minutes setting it up. I might pull this into a library in the future.