FactoryBoy / factory_boy

A test fixtures replacement for Python
https://factoryboy.readthedocs.io/
MIT License
3.49k stars 392 forks source link

Model factory should load model as late as possible #160

Open maiksprenger opened 10 years ago

maiksprenger commented 10 years ago

Django uses string notation in several places (e.g. ForeignKey) to be able to reference a model during the app startup phase. Factory boy recently added that syntax, but unfortunately due to the way it's implemented, the original purpose of lazy loading the model is defeated.

The model class is loaded in the factory's Meta class __new__ method. That means at import time, not when the factory is first instantiated. The model class is loaded via Django's get_model method. That means using the string syntax is actually worse than directly declaring the model, because at least on Django 1.7, get_model fails loudly if the entire app cache isn't populated yet, while passing the model lets Django deal with the population of the app cache.

This leads to all kinds of app startup problems when e.g. collecting tests for a test suite, because it suddenly requires the app cache to be fully initialised. But while directly referencing the model as a workaround is possible, it can still lead to tricky situations.

One way to fix it might be to only load the model class when the factory itself is initialised.

rbarrois commented 10 years ago

Hi,

That's indeed a nasty bug; I'll look into it soon.

maiksprenger commented 9 years ago

@rbarrois, did you get a chance to look at this? Do you think it's easy enough to fix by an outsider like me? Otherwise I can have a stab at a pull request.

rbarrois commented 9 years ago

Well, it should be much easier now that I have removed the automatic sequence setup from __new__.

If you can prepare a PR (ideally with a test), I should have the time to merge it this week.

Thanks!

gaffney commented 9 years ago

Hello @maikhoepfel

Any update on this?

maiksprenger commented 9 years ago

Gaffney, I'm afraid I'm pretty swamped at the moment; my OSS contributions are taking a bit of a hit at the moment. I did investigate for a bit though. We had a workaround for this issue, and I could remove it without triggering any problems. That made me dig into the code, as @rbarrois mentioned having done some changes. But as far as I can see, the problem is still there. FactoryMetaClass.__new__ still calls contribute_to_class, which triggers a call to get_model. But I have to admit I don't understand what exactly @rbarrois changed. Is this still a problem for you, @Gaffney?

Writing a test against model loading is rather fiddly, because by the time the test suite runs, Django will have already loaded all the models...

rbarrois commented 9 years ago

Indeed, I can confirm that the bug is still here. I see a few options here: