Make tests order independent #932

rhizoome · 2020-04-14T14:54:37Z

Reset the global registry after each test (teardown)
Create a settings fixtures that returns graphene_settings and resets the graphene_settings after use (teardown)
Convert test_mutation tests from unittests.TestCase to pytest
Convert test_mutation PetType to a pet_type fixtures that reregisters the type

The core change is setup/teardown in conftest.py

graphene-django/graphene_django/conftest.py

Lines 8 to 18 in a16cebe

    
           @pytest.fixture(autouse=True) 
        
           def reset_registry_fixture(db): 
        
               yield None 
        
               reset_global_registry() 
        
           @pytest.fixture() 
        
           def settings(): 
        
               settings = dict(graphene_settings.__dict__) 
        
               yield graphene_settings 
        
               graphene_settings.__dict__ = settings

jkimbo · 2020-04-16T13:31:57Z

@ganwell if you need to drop Python 2.7 support then why not create this PR against the v3 branch?

rhizoome · 2020-04-16T14:15:30Z

@jkimbo I rebased to v3 and removed WIP

jkimbo

Looking good @ganwell . Left some comments

jkimbo · 2020-04-16T14:45:47Z

graphene_django/conftest.py

+    settings = dict(graphene_settings.__dict__)
+    yield None
+    reset_global_registry()
+    graphene_settings.__dict__ = settings


Why do you have to restore the graphene_settings dict?

There are tests that change the settings and don't change it back.

There are several options:

Provide a graphene_settings fixture (probably the best way). This fixture resets the settings after use

Just reset the settings on every test, so it is impossible to leak any settings. I decided to use this approach because it is best for an environment where contributors often change.

Manually reset the settings in the tests

We usually use 1. but we have regular contributors. Let me know what you prefer.

Uses of graphene_settings:

$> git grep graphene_settings | grep '/tests/' graphene_django/forms/tests/test_mutation.py:from ...settings import graphene_settings graphene_django/forms/tests/test_mutation.py: graphene_settings.CAMELCASE_ERRORS = True graphene_django/forms/tests/test_mutation.py: graphene_settings.CAMELCASE_ERRORS = False graphene_django/rest_framework/tests/test_mutation.py:from ...settings import graphene_settings graphene_django/rest_framework/tests/test_mutation.py: graphene_settings.CAMELCASE_ERRORS = True graphene_django/rest_framework/tests/test_mutation.py: graphene_settings.CAMELCASE_ERRORS = False graphene_django/tests/test_converter.py:from ..settings import graphene_settings graphene_django/tests/test_converter.py: graphene_settings.DJANGO_CHOICE_FIELD_ENUM_V3_NAMING = True graphene_django/tests/test_converter.py: graphene_settings.DJANGO_CHOICE_FIELD_ENUM_V3_NAMING = False graphene_django/tests/test_query.py:from ..settings import graphene_settings graphene_django/tests/test_query.py: graphene_settings.RELAY_CONNECTION_ENFORCE_FIRST_OR_LAST = True graphene_django/tests/test_query.py: graphene_settings.RELAY_CONNECTION_MAX_LIMIT = 100 graphene_django/tests/test_query.py: graphene_settings.RELAY_CONNECTION_ENFORCE_FIRST_OR_LAST = False graphene_django/tests/test_query.py: graphene_settings.RELAY_CONNECTION_MAX_LIMIT = 100 graphene_django/tests/test_query.py: graphene_settings.RELAY_CONNECTION_ENFORCE_FIRST_OR_LAST = False graphene_django/tests/test_types.py:from ..settings import graphene_settings graphene_django/tests/test_types.py: graphene_settings.DJANGO_CHOICE_FIELD_ENUM_V3_NAMING = True graphene_django/tests/test_types.py: graphene_settings.DJANGO_CHOICE_FIELD_ENUM_V3_NAMING = False graphene_django/tests/test_types.py: graphene_settings.DJANGO_CHOICE_FIELD_ENUM_CUSTOM_NAME = ( graphene_django/tests/test_types.py: graphene_settings.DJANGO_CHOICE_FIELD_ENUM_CUSTOM_NAME = None

I think a fixture would be good as well. If it's going to be too much work to change it all though I'm ok leaving it like this.

jkimbo · 2020-04-16T14:48:09Z

graphene_django/forms/tests/test_mutation.py

-    text = forms.CharField()
+@pytest.fixture()
+def my_form():
+    class MyForm(forms.Form):


Why do the forms need to be fixtures?

@jkimbo I think only the types have to be fixtures, because they get registered in the global registry through some magic when python executes the class. I put everything into fixtures just for consistency.

If you like I can check what needs to be fixtures and only more these to fixtures.

I think that would be cleaner. As far as I'm aware only the Django models and Graphene-Django DjangoObjectType's register themselves into a registry.

jkimbo · 2020-04-16T14:49:16Z

graphene_django/forms/tests/test_mutation.py

+    assert result.errors[0].messages == ["This field is required."]
+    assert result.errors[1].messages == ["This field is required."]
+    assert "age" in fields_w_error
+    assert "name" in fields_w_error


Much prefer plain pytest tests rather than Django's TestCase 👍

jkimbo · 2020-04-16T14:50:11Z

pytest.ini

@@ -1,2 +1,3 @@
 [pytest]
 DJANGO_SETTINGS_MODULE = django_test_settings
+addopts = --random-order


Pytest config shouldn't be defined in multiple places. I would remove the pytest.ini file and just define it in the setup.cfg file.

rhizoome · 2020-04-16T15:58:03Z

@jkimbo I think I addressed all the issues.

rhizoome · 2020-04-17T09:23:13Z

@jkimbo I will split this PR into one that can be merge to master and the rest that can only be merged to v3. In about 2 hours.

jkimbo · 2020-04-17T12:24:18Z

If you feel it's important @ganwell . I'm fine with just merging this into v3

rhizoome · 2020-04-19T18:20:44Z

@jkimbo I moved this PR to master again. It now contains everything except adding pytest-random-order and can therefore be merged to master.

rhizoome · 2020-04-19T18:34:46Z

@jkimbo Here is the part that only works on v3 11e1403 / #940

Sorry for making it so complicated I felt it is the right thing to do in case it takes longer to merge v3 to master.

I will checkin on #932 and #940 the next days. In case you think it is all too complicated just do what you think is best: ie commiting it yourself.

jkimbo

This is looking good thanks, @ganwell . Just one thing before I think we should merge this.

jkimbo · 2020-04-19T19:46:39Z

graphene_django/conftest.py

+
+
+@pytest.fixture()
+def settings():


Suggested change

def settings():

def graphene_settings():

Otherwise this will clash with the pytest django settings fixture: https://pytest-django.readthedocs.io/en/latest/helpers.html#settings

* Reset the global registry after each test (teardown) * Create a settings fixtures that returns graphene_settings and resets the graphene_settings after use (teardown) * Convert test_mutation tests from unittests.TestCase to pytest * Convert test_mutation PetType to a pet_type fixtures that reregisters the type

rhizoome · 2020-04-20T10:13:17Z

@jkimbo, I changed the fixture settings() to graphene_settings()

When you merge, could you please also merge master into v3 again, so I can update #940? It currently includes a merge, which makes it difficult to read the diff.

jkimbo · 2020-04-20T12:25:02Z

@ganwell v3 updated

rhizoome mentioned this pull request Apr 14, 2020

☂️Graphene-Django v3 #705

Closed

7 tasks

rhizoome changed the title ~~Make tests order independent~~ WIP: Make tests order independent Apr 16, 2020

rhizoome changed the base branch from master to v3 April 16, 2020 13:59

rhizoome changed the title ~~WIP: Make tests order independent~~ Make tests order independent Apr 16, 2020

jkimbo requested changes Apr 16, 2020

View reviewed changes

rhizoome marked this pull request as draft April 17, 2020 09:22

rhizoome changed the title ~~Make tests order independent~~ WIP: Make tests order independent Apr 17, 2020

rhizoome changed the base branch from v3 to master April 19, 2020 17:48

rhizoome marked this pull request as ready for review April 19, 2020 18:19

rhizoome changed the title ~~WIP: Make tests order independent~~ Make tests order independent Apr 19, 2020

jkimbo requested changes Apr 19, 2020

View reviewed changes

jkimbo approved these changes Apr 20, 2020

View reviewed changes

jkimbo merged commit b9f0e4f into graphql-python:master Apr 20, 2020

rhizoome mentioned this pull request Apr 29, 2020

graphql 3.0 and graphene 3.0 final rebase #951

Merged

	@pytest.fixture(autouse=True)
	def reset_registry_fixture(db):
	yield None
	reset_global_registry()


	@pytest.fixture()
	def settings():
	settings = dict(graphene_settings.__dict__)
	yield graphene_settings
	graphene_settings.__dict__ = settings

Make tests order independent #932

Make tests order independent #932

Uh oh!

Conversation

rhizoome commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkimbo commented Apr 16, 2020

Uh oh!

rhizoome commented Apr 16, 2020

Uh oh!

jkimbo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhizoome commented Apr 16, 2020

Uh oh!

rhizoome commented Apr 17, 2020

Uh oh!

jkimbo commented Apr 17, 2020

Uh oh!

rhizoome commented Apr 19, 2020

Uh oh!

rhizoome commented Apr 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkimbo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhizoome commented Apr 20, 2020

Uh oh!

jkimbo commented Apr 20, 2020

Uh oh!

Uh oh!

rhizoome commented Apr 14, 2020 •

edited

Loading

rhizoome commented Apr 19, 2020 •

edited

Loading