search

laracasts / TestDummy

Easy factories for PHP integration testing.
https://laracasts.com/lessons/whats-new-in-testdummy
MIT License
456 stars 80 forks source link

Bug reintroduced with recent commit #78

Closed devinfd closed 9 years ago

devinfd commented 9 years ago

mergeFixtureWithOverrides was updated to use array_merge with the attributes. This was the original behavior in v1.0 and resulted in throwing Illuminate\Database\QueryException Unknown column errors. This bug was fixed in https://github.com/laracasts/TestDummy/pull/29 but it was recently changed back in https://github.com/laracasts/TestDummy/commit/e21fedc5ed56922549e6a4926b24be8f4b3f8ead to "Allow overriding fields not defined in a factory". For me, the better solution to that problem would be to just add the fields to the factory.

This PR reverts back to the working version.

fixes: https://github.com/laracasts/TestDummy/issues/77

laracasts commented 9 years ago

Honestly, I don't have any real opinion on this. Haven't noticed issues with either approach.

@adamwathan Any thoughts on this PR being merged?

devinfd commented 9 years ago

Just an fyi, if this PR isn't accepted then attributes can't be passed to relationships and https://github.com/laracasts/TestDummy/pull/52 would become irrelevant.

bobbybouwmann commented 9 years ago

Regarding to #77, when I removed the attributes everything worked fine, but with attributes the relations can't be created correctly

adamwathan commented 9 years ago

There must be a way to preserve both behaviors, gonna dig in to it a bit now.

The change was triggered by this issue which gives some more detail: https://github.com/laracasts/TestDummy/issues/64

My intention is just to lean on the wisdom of the maintainers of Factory Girl, who started using the pattern in 2008.

From the docs:

It is highly recommended that you have one factory for each class that provides the simplest set of attributes necessary to create an instance of that class. If you're creating ActiveRecord objects, that means that you should only provide attributes that are required through validations and that do not have defaults.

The goal of a factory is to define the minimum attributes necessary to have database-valid instance of that class. If you have to define attributes that are nullable or have defaults provided by the database, you have introduce duplication of knowledge for no real reason.

Anyways, going to try and come up with a solution that allows both. It shouldn't be necessary to define every attribute to support a different feature.

devinfd commented 9 years ago

After reading define the minimum attributes necessary to have database-valid instance of that class I think I have a better understanding of where you are coming from with Allow overriding fields not defined in a factory https://github.com/laracasts/TestDummy/commit/e21fedc5ed56922549e6a4926b24be8f4b3f8ead. You are keeping the factories lean as to not overcomplicate them right?

I'm all for simplicity but I also think that a factory that defines every attribute is more explicit and better "documents" what the modal is actual all about. This makes it easier for future devs or myself months later from now.

...now I'm not sure what the best solution is. Simplicity or explicitly....

adamwathan commented 9 years ago

It's not so much keeping them lean to avoid overcomplicating, I think it's more about what I think a test factory is meant for.

To me, the job of a test factory is "let me easily instantiate an object that is just valid enough that I can only worry about setting the attributes that matter to my test and everything else will just work" you know what I mean?

I totally understand what you want and I agree it's a useful feature, I just think we need to define a more explicit syntax.

adamwathan commented 9 years ago

@devinfd I've opened an issue to discuss coming up with a better syntax for overriding attributes in relationships here: #82

Would love your input!