implement django tests with custom runner

[eleanorjboyd]

This PR is a working demo of how use a custom runner for Django which defines kwargs["resultclass"] = UnittestTestResult that allows for the results to get processed and sent exactly like unittest. This also allows for django tests to be run with python manage.py test so that setup and teardown work. This is an alternative solution to #21468 which allows the extension to run Django tests exactly like the command line.

This is still an experiment, much more analysis will have to be conducted into if this is a feasible solution.

[mh-firouzjah]

vscode-python/pythonFiles/unittestadapter/django_runner.py

Line 61 in c59eb26

with open(settings_file, "w") as f:

This change will be permanent in the settings.py module, and on the other hand, I don't think it's a good idea for a plugin to allow itself to make such a change in one of the most important project files.
Additionally, even reading information from this module may challenge your plugin in terms of security.

• It's possible to use --testrunner flag for manage.py test to avoid applying such a change in settings.py module
  • --testrunner TESTRUNNER:
    Tells Django to use specified test runner class instead of the one specified by the TEST_RUNNER setting.

[eleanorjboyd]

Aha! Fabulous, I must have missed the --testrunner flag. So you are saying if I use this flag then the security concerns would be reduced? Want to make sure I understand your concern correctly. Thanks!

[eleanorjboyd]

hello @mh-firouzjah, I have switched to using --testrunner per your suggestion and it works great! Any other feedback you already have or have you been able to try it yourself to see if it works as expected? Wondering if you think this is a better solution than what we originally talked about and if I should close the previous PR. Let me know your opinion, thanks!

[eleanorjboyd]

a space as the join argument? I think I already put one in between the " but ill check

[mh-firouzjah]

I reviewed the changes here, and what I understood from the code:

• we have CustomTestRunner which is a subclass of DiscoverRunner so it's able to both discover and run django tests.
• inside the CustomTestRunner you used the get_test_runner_kwargs to then pass the UnittestTestResult as a kwarg parameter and this class could hold the execution results of the instance of CustomTestRunner.
• that class CustomTestRunner has been used inside the django_execution_runner function which itself will be invoked when an environment variable indicates that django_test_enabled and django_test_enabled.lower() == "true".

Awesome! Good job. But if these changes are not just to execute tests but also to discover tests as well(regarding the separation of these two jobs in the extension till now), as far as I know, the DiscoverRunner from Django does the ability to discover the tests but this capability is used in other ways which I can't see you're taking advantages of it.
because DiscoverRunner will execute discovered tests immediately so it wont help to make discovery a separate step. I'm sorry for being so long-winded.

[mh-firouzjah]

I was working on this patch. but reviewing your code here changed my mind.

According to what I had there and what you brought here:

• What I was working on, had some unnecessary steps which your code simply fixed those.

• The DiscoverRunner has a built in method build_suite which is used for discovering tests.

• You can use build_suite method for the discovery part as well - a solution for what I mentioned in previous comment.
  Also unittest is capable to discover django tests, but it requires setup_django_env or similar action at first place I'm wondering if we could surpass that. according to django's implementation we can have one more subclass of DiscoverRunner with the same idea of overriding get_test_runner_kwargs, but inside of which we override run_tests so it will just invokes build_suite and returns the result.
  Like what it already is here but stop at the mentioned line and don't go further down:

  class DjangoTestDiscoverer(CustomTestRunner):
      """Inherited From Your CustomTestRunner"""
      def run_tests(self, **kwargs):
          """
          Overridden to just make discovery of tests possible
  
          Return the discovered tests.
          """
          self.setup_test_environment()
          return self.build_suite(test_labels)

  Now if we call manage.py test --testrunner DjangoTestDiscoverer it will take care of setting up django environment and it will discover all tests but wont execute them.

  *But this implementation doesn't use UnittestTestResult so there must be a way to take care about the result. maybe there should be a separate directory for djangotestadapter only? along side with unittestadapter and vscode_pytest.

• The idea of using environment variables to indicate that the extension shall be ready for Django Tests (django_test_enabled and django_test_enabled.lower() == "true" and so on), could be replaced with a more harmonic way of the same procedure of having an option in preparing test environment for the first time - in which a drop-down menu asks users to select either of pytest/unittest and some other questions to make the extension ready to discover and run tests. - in this case adding an option for django_tests and asking some more questions to get raid of any ENV Variable, is my suggestion.

[eleanorjboyd]

Thanks for your expertise! Ill look into the build_suite method for the discovery part as well!

In terms of the UI, we are planning on having a way similar to what it is like now to setup Django testing. We are moving to a new style of how testing args work as proposed / discussed here: #21845. With this change we might be changing around our menu flow but I was thinking it would look something like this:

click configure tests -> dropdown of either pytest or unittest (but unittest includes a descriptor like "use unittest to config django tests") -> click on unittest -> dropdown of either basic unittest or Django tests -> user selects Django test it would be configured in their settings as an environment variable that therefore the person could change later but the flow would be similar.

See below for how the Django env var would then show up in our proposed config json design:

{
      "name": "Django run config", 
      "args": ["-v"],
      "env": {
	"PYTHONPATH": "${workspaceFolder}",
        "DJANGO_TESTING": "true",
	"DJANGO_MANAGE_PY_PATH": "/path/to/file",
      },
},

[mh-firouzjah]

hi👋
Thanks to your explanation I got the idea of how you're going to use GUI to let people set up for Django tests.

Now it seems because surly there will be a manage.py module, the two steps of discovery and running can be done easier:

• For the run part I like what you've done, that's clean and concise

• For the discovery, I've tried some ideas and a working solution is to set DJANGO_SETTINGS_MODULE and call django.setup(). This will let the default/current unittest implementation in the extension to discover Django tests.

  • Setting django environment up before the unittest starts the discovery, as I have already tried this, the change have to be take place in both pythonFiles/testing_tools/unittest_discovery.py and pythonFiles/unittestadapter/discovery.py modules. I had a function setup_django_env.
  • DJANGO_SETTINGS_MODULE could be declared in as an Environment Variable either using for example a .env file or in addition to manage.py module, it could be selected via GUI or be filled in relative json file. In this case the missing part is to call django.setup() before the unittest starts discovery:

  def setup_django_env():
    try:
        import django
        django.setup()
    except Exception as e:
        print(f"Error configuring Django test runner: {e}")
        raise VSCodeUnittestError(f"Error configuring Django test runner: {e}")

  • If that's not the case, then we have to read the contents of manage.py module to get the DJANGO_SETTINGS_MODULE and set the environment variable and then call to django.setup().

Notice: test_list discovered by unittest is not acceptable arg for DiscoverRunner.run_tests, You're not doing so then there's no issue.

[eleanorjboyd]

Hello again! Your suggestion for setting the DJANGO_SETTINGS_MODULE and call django.setup() sounds good to me. As I am not very familiar with Django can you provide an example where Django setup is required for test discovery? I am still trying to wrap my head around what setup provides for the actions taken during discovery.

Secondly I updated the proposal to feature what we have discussed here. I would appreciate any comments you could add there on our discovery discussion as I did not include that just yet and wanted to instead continue our discussion on it either here or in that thread.

Finally, I wanted to give you a bit of a timeline on this specific project. Due to scheduling, including a personal vacation and then housekeeping work in December I will not be able to make progress on this for the reminder of the calendar year. Additionally, since I want to implement this using env vars, those are currently not configurable with testing as of the rewrite and will require this proposal to be finalized and implemented. Once that is done we can then expose env vars for people to set and to enable this Django testing design. We are not sure on the timeline for the testing args proposal but it is a substantial change that will require input from many stakeholders and therefore do not think it would be done before March of next year. I cannot promise any dates but wanted to give you a heads up about the timeline and that this feature, as it is dependent on the proposal, will not be completed for some time. I am sorry about this as I know Django testing has been highly requested for a while but appreciate your patience in the next few months as we look to make architecture changes that will enable us to add Django support. Thanks

[mh-firouzjah]

where Django setup is required for test discovery?

Calling django.setup() is required for “standalone” Django usage
Also from the old discussion look at this and more noticeable one from carltongibson.
And also ignoring to use django.setup() will result in django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet. exception.
And as @carltongibson explained here:

the basics are setting the DJANGO_SETTINGS_MODULE environment variable, and then calling django.setup().

Since in the last shot, you fixed the test running problem by using a custom test-runner, I think what @carltongibson mentioned about taking care of database creation and tearing-down and so on, will be covered behind the since(well, it is basically what they did themselves: manage.py test).
While using manage.py test will free us of taking care of any of those steps in the running process, but in the discovery process no manage.py test is used:

just from a Django perspective, calling django.setup() is going to be needed. It requires an import path to a settings module, either via DJANGO_SETTINGS_MODULE, env var or a --settings CLI flag to django-admin. (The default manage.py file sets a default for DJANGO_SETTINGS_MODULE to the settings file created with the startproject command, so that's not a bad fallback value.)

[eleanorjboyd]

makes sense! Thanks for your reply and therefore we will use your original recommendation around setup for discovery!

[dark0dave]

Has there been any progress on this? Without this debugging testing process is currently painful. It would be useful to have this :)