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

Fixtures silently do nothing when --simple is specified, and no documentation calls this out #114

Open
LucidDan opened this issue Jul 27, 2020 · 10 comments

Comments

@LucidDan
Copy link

While learning to use behave and behave-django, having come from a pytest background, I had very little pre-existing knowledge of Django's testcase implementations. One thing that tripped me up was that it was my fixtures were not being loaded - I was using --simple because I don't need or want the liveserver, but it was not immediately obvious why things were failing, until I did some digging in the code and the Django documentation.

I wouldn't call this a bug, but I think it isn't great for something to silently fail, or if that's a reasonable outcome it should definitely be documented, even if only briefly.

Some possible solutions I can see:

  1. Improve documentation with a "Note" box in the fixtures section and/or in the --simple section, explaining that SimpleTestCase doesn't support some functionality like loading fixtures.
  2. Add warnings (or errors? probably not, I guess) when using --simple and fixtures in combination, warning that fixtures won't have any effect and may cause unexpected behaviour.

Let me know what you think, happy to do a PR for one or the other of these. I have some other thoughts on the issue of testcase type being used, but will put that in a separate ticket as I think it is a totally separate issue from the documentation and silent failure of fixtures.

@LucidDan
Copy link
Author

LucidDan commented Jul 27, 2020

I just noticed that there is a RuntimeError raised if you use context.base_url when specifying --simple, so perhaps an error if you have any fixtures specified while using --simple is consistent with that, after all? Although I guess it might be acceptable to not have fixtures loaded. I'm not entirely clear at this point on what the value of --simple is at all, in the first place. Maybe that's something I've missed, or the better solution is to more clearly state the intended use case(s) for it in the documentation.

@bittner
Copy link
Member

bittner commented Aug 3, 2020

I'm not sure why using --simple should result in a different behavior with fixtures. For what I understand from the documentation and source code there should be no reason why fixtures aren't loaded.

Related resources:

Were you able to understand more? Would you be able to add a test case that mimics the behavior?

@bittner
Copy link
Member

bittner commented Aug 3, 2020

For some reason, the tests that cover fixture loading specify a @requires-live-http decorator.

@sebastianmanger, @mixxorz, does one of you happen to remember why?

@bittner
Copy link
Member

bittner commented Oct 19, 2020

@sebastianmanger @mixxorz ping 🔔

@bittner
Copy link
Member

bittner commented Nov 30, 2020

Huuu-huuu, @sebastianmanger! 🎅 Ho-ho-ho, @mixxorz! 🤶 Any idea how we can explain the behavior? 🔔

@LucidDan
Copy link
Author

ooops sorry @bittner , I completely missed the August message, I'm setting up a new project with behave-django now, so I'll go back and review the issue I had and get back to you with a practical example of what I encountered.

@rodrigondec
Copy link

@bittner @LucidDan I understand why.

I struggled a bit with the behave-django untill I undestood that you guys decided to use StaticLiveServerTestCase as the default test case suit instead of TestCase.

The --simple argument makes the test suit TestCase be used.

Django's test are robustly made. It has different facets.

First thing's first: Django behavior from those suit

StaticLiveServerTestCase has a different implementation from TestCase.

BOTH inherit from TransactionTestCase. Which means BOTH tests from those suits runs inside a atomic transaction block.

Both has the following _post_teardown behavior

        """
        Perform post-test things:
        * Flush the contents of the database to leave a clean slate. If the
          class has an 'available_apps' attribute, don't fire post_migrate.
        * Force-close the connection so the next test gets a clean cursor.
        """

But TestCase has an additional behavior that StaticLiveServerTestCase doesn't have. The rollback_atomics with the following code

    @classmethod
    def _rollback_atomics(cls, atomics):
        """Rollback atomic blocks opened by the previous method."""
        for db_name in reversed(cls._databases_names()):
            transaction.set_rollback(True, using=db_name)
            atomics[db_name].__exit__(None, None, None)

Consequence of using StaticLiveServerTestCase

Between your tests the test suit WON'T roll back the atomics!

Your only behavior is from TransactionTestCase which leaves you with a database with clean state (as stated on TransactionTestCase documentation).

What does that mean?

Any initial data loaded in migrations will only be available in TestCase tests and not in TransactionTestCase tests, and additionally only on backends where transactions are supported (the most important exception being MyISAM). This is also true for tests which rely on TransactionTestCase such as LiveServerTestCase and StaticLiveServerTestCase.

https://docs.djangoproject.com/en/3.2/topics/testing/overview/#rollback-emulation

And this was a problem for me. Which I will explain bellow.

My project and why I struggled with behave-django

I work at Imobanco. A fintech.

Our project uses Django. It's big and complex.

We have multiple migrations (both structural and data).

Besides that, our software need some "fixtures" to run too. But we didn't implement them as fixtures, instead we worked with implementation on the test suit using factories...

hence my motivation for #122 and #123

As I mentioned, we use the TestCase and not the StaticLiveServerTestCase on our 1000+ unit tests.

Our DB is created from migrations WITH initial data (from data migrations) and them is "fixtured" with factories on the Test Suit level (TestCase).

When we used the default command CLI for behave-django our test implementation failed due to missing data. Crucial missing data which should have been populated from migrations (it was from a data migration).

And only the SECOND behave test failed. The first passed. So we questioned, why? Why does the first test have a functional DB (with migrated data and so on) while the second doesn't?

The symptom was evident. Test isolation. But why did the test isolation fail?

That's when I went digging for the test suit used by default on behave-django.

The solution for my problem was easy. Always use the --simple flag 🤣

In conclusion

Yes, you guys should document it in a more detailed way in the section https://behave-django.readthedocs.io/en/stable/isolation.html.

In this page it has the phrase:

Each scenario is run inside a database transaction, just like your regular TestCases

This phrase is falsy. Your DEFAULT test isolation it's not like the regular TestCases.

And you don’t have to clean the database yourself.

This phrase is truthy. But it's also true that in your DEFAULT test isolation, it's need to populate and recreate de database yourself.

In this section should be documented the differences between the default StaticLiveServerTestCase used and the django standard TestCase used when invoked with --simple arg!

@bittner
Copy link
Member

bittner commented Oct 3, 2021

So, the behavior of --simple is buggy and we need to fix it, is this correct? Let me summarize:

  1. We want --simple to behave the same way as "non-simple" (apart from spinning up a LiveServer, in addition).
  2. We want fixtures to be loaded by Django for both --simple and "non-simple" (the same way).
  3. We want test isolation with scenarios, hence database changes rolled back at the end of any scenario (for both --simple and "non-simple").

We want things to be consistent and easy to reason about.

Can one of you create test cases in our test suite that show - failing - that the current implementation does not support this consistency? Then we can change the implementation accordingly, afterwards.

@phitranphitranphitran
Copy link

A workaround that got --simple working with fixtures for me - instead of setting context.fixtures, call loaddata during before_scenario

from django.core.management import call_command

def before_scenario(context, scenario):
    fixtures = [...]  # what you would have put in context.fixtures
    call_command("loaddata", *fixtures, verbosity=0)

@bittner
Copy link
Member

bittner commented Oct 14, 2022

I added a note to the project's documentation, as the suggested. Visible in the latest docs for now, until the next release.

EDIT: This shouldn't be the end of the story. Ideally, behave-django should be intuitive and logical out-of-the-box. There should be no surprises, not for newcomers and not for Django experts. If you have an idea of a great fix please explain it here or start a PR directly. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants