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

Enable FastFixtureTestCase on Django 1.8+ #258

Closed
wants to merge 6 commits into from

Conversation

bennylope
Copy link

This changeset allows the FastFixtureTestCase to be used on Django 1.8+.

This is not a rewrite or a comprehensive set of tests for the test case class. It is however an incremental move forward testing a baseline, can FastFixtureTestCase be used in tests? The first commit adds the basic test - which will fail - and next three, specifically 8cb7e91 apply the changeset to pass.

I'm sure the FastFixtureTestCase could use some additional work, certainly additional testing, getting it working seems like a good start.

Also solves the issue in #200 where no fixtures are specified and the test case fails (a necessary fix for the application I'm supporting).

Does not solve problems specifically for #162 or #208 as those issues reference now unsupported versions of Django. I'd reckon those issues could probably be closed since they're out of date now.

Sites are referenced in the tear down for FastFixtureTestCase
In the scenario that a test file does something like::

    from django_nose.testcases import FastFixtureTestCase as TestCase

And then has a bunch of fixture related test cases but *one* test case
that doesn't have fixtures - because why import another test class for
that one little test case? - in that scenario the testcase will raise
an error iterating over a NoneType. Ensures that fixtures are only
handled when actually present.
@zdelagrange
Copy link

any issues with this PR?

@jwhitlock
Copy link
Contributor

Only that I avoid FastFixtureTestCase because it is buggy, and so haven't tested the patch. Have you tested for your use cases?

@zdelagrange
Copy link

currently attempting to implement the changes. does this PR need test coverage? what will make you confident?

@jwhitlock
Copy link
Contributor

I'd like someone that actually uses FastFixtureTestCase in their project to confirm that it works with their projects' tests. I don't think there's any change that would tempt me to use them in my own code.

@bennylope
Copy link
Author

I've been using this patch with a project running Django 1.8.12 since creating the pull request. Would it be worthwhile adding warnings for the TestCase's execution, i.e. that the FastFixtureTestCase is largely unsupported or is untested legacy code?

I totally understand why you're not eager to include this. In my case the project using this feature has a legacy test suite that would take a significant effort to rewrite, relies on large fixture files, and originally took over 90 minutes to run in full (now under 60 seconds after trebling the number of tests).

@jwhitlock
Copy link
Contributor

@bennylope, I'm glad it works for you. I need a second project to test it out to confirm that the fixes are at least applicable to one other codebase.

@lokeshatbigbasket
Copy link

Hi @bennylope ,

I have a question with this PR. Does removing transaction completely solves the problem ??? I think that is for setting up database right??/

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

Successfully merging this pull request may close these issues.

None yet

4 participants