Fixed Django tests discovery

[mh-firouzjah]

By invoking newly added function setup_django_test_env it will try to set up django environment by setting DJANGO_SETTINGS_MODULE environment variable and invoking django.setup method, if it succeeds django tests will be discovered as other tests currently are, otherwise it fails silently.

Currently the extension will fail because it doesn't set the required environment variable and also it doesn't invoke the django setup function, so make the environment ready for django tests.

I've added that function which tries to import django if it success then will try to find manage.py file which is a must have file for any django project and which is generated by django-admin startproject command. inside this file there is an attempt to set the DJANGO_SETTINGS_MODULE environment variable, and my function tries to extract the value of this variable from this file.
at the end of the day, it will set the DJANGO_SETTINGS_MODULE environment variable and invokes the django.setup function, if it succeeded the environment is prepared for django tests so the extension will no longer fail and we are good. if it fails to import django or find that file, the function will fail silently and has no effect on the extensions normal processes.

This is the result of this change:

[mh-firouzjah]

@karthiknadig @eleanorjboyd
I hope this message finds you well. I wanted to request your initial approval for my pull request so that the GitHub actions and workflows can start processing my changes.

I have thoroughly tested and reviewed the changes I made to ensure they meet the project requirements and adhere to the coding standards. I believe that my changes will benefit the project and I would appreciate your approval.

Thank you for taking the time to review my pull request. I look forward to hearing your feedback.

[eleanorjboyd]

Hello @mh-firouzjah! Apologies for the delay in getting back to you on this! Thank you for contributing and the detail you put into this PR, that is very much appreciated! As you know from this discussion which you have been a part of, the team has been thinking about the best way to provide Django support. My goal here is to provide the solution that works for the most number of people. Due to the fact that I am still a beginner in Django specifics, I have reached out to my colleague with Django expertise and will circle back. Thank you again, we rely on people in the community like you to make our extension useful for everyone!

[mh-firouzjah]

Hi @eleanorjboyd, appreciations.
Thank you for your efforts in finding a solution for providing Django support and for collaborating with colleagues who have more experience in Django. It's reassuring to see that you're taking steps to ensure that the solution meets the needs of the community. I appreciate your updates on the progress and for the opportunity to contribute to the extension.

[eleanorjboyd]

Hello again! I have discussed with my colleagues and think this is the flow we want to have for the setup/ discovery:

1. create new argument that can be put in the "python.testing.unittestArgs": [],
   1. the arg will be called something like djangoSetupScriptPath and it will be the path to a script they want to use to handle setup if they do not want to use the default
      1. for example "python.testing.unittestArgs": ["-djangoSetupScriptPath", "path/to/file"],
      2. if there is no script provided it will default to django_test_init.py which will be this same functionality you use to load Django but in a separate script (more on this later)
2. unittest discovery is run
3. on unittest discovery, pass arg -djangoSetupScriptPath like all other ags if it exists
4. from unittest script (so inside the spun up python server env) parse args and save -djangoSetupScriptPath if it exists
5. call import django to see if it works
   1. if it doesn’t → fail quietly
   2. if it does: →
6. then call the script provided (if -djangoSetupScriptPath existed) or the default script
   1. we want this script to be imported correctly so add to path as necessary to make sure we can reach the given script (based on path provided if not default)
   2. the default script will search and find those basic setup args in the manage.py (generally what you already have for setup but in a seperate script).

In terms of execution, about the same idea for that just replicating some imports and calling the same logic to parse args, try importing and run the script.

How does this sound to you? It just makes it a bit more configurable where the extension provides the simplest option to setup Django with a default script but users can make their own if they want more control / need more setup.

Also we are just now moving over to an architectural rewrite (it will go out on stable very shortly). You have most of your code in the new sections, we will no longer be using anything in the testing_tools folder or the file pythonFiles/visualstudio_py_testlauncher.py. All your code should go in /unittestadapter/discovery.py or /unittestadapter/execution.py. Also arg parsing is done in the function parse_unittest_args, you can look at how other args are parsed to do the same for this new one also feel free to give feedback on what to call this arg

Sorry, this is a lot of info at once! Let me know your thoughts, ideas, or questions. Thanks!

[mh-firouzjah]

Thank you for explaining your requirements. Based on your explanation, I have reviewed the code and implemented the changes in a new way.

• To clarify, there is no need for lengthy code like what I wrote or expect such a code from users, to be able to set up django environment for django tests.
  what I did at my previous commit was to find out where is the path to the main settings file of the django project the user is working on.
  • This file may not necessarily be named settings.py so I was looking for that inside the mandatory manage.py file instead.

The function in django_test_init.py expects two arguments. The first argument is django_settings_module, which is the string path to the Django project's main settings file and is used for the DJANGO_SETTINGS_MODULE environment variable. The second argument is called root, which is the starting directory to where the possible Django project started in. The second argument is optional, and we can discuss later whether it should be kept or not.

• If django_settings_module is not provided, I still use root to determine if we have a Django project or not, as I did in my previous commit.

I also noticed your parse_unittest_args function and used it to check if django_settings_module has been specified or not. I passed django_settings_module to setup_django_test_env to set up the Django test environment, just as you suggested.

start_dir, pattern, top_level_dir, django_settings_module = parse_unittest_args(argv[index + 1 :])

# Setup django env to prevent missing django tests
if django_settings_module is not None:
    setup_django_test_env(django_settings_module)
else:
    # Try to be smart and find DJANGO_SETTINGS_MODULE
    setup_django_test_env(root=start_dir)

• Here the else part is what I want to discuss about it more, this part and also inside the setup_django_test_env the part which is using root (the try/except block) tries to be smart to find out what is DJANGO_SETTINGS_MODULE could be removed since we can expect that if we have a Django project, it will be cleared when django_settings_module is specified. but I would like to keep this code there since it is safe and harmless. It can recognize and prepare a Django test environment or fail quietly, so even if django_settings_module is not specified by any mistake there is a chance to be prepared for possible django environment.

The important part which is not clear to me is how we are going to pass django_settings_module as a command line arg to parse_unittest_args and how you already are passing those other args there?

The second question is, there is a test option in side menu of vscode which if user clicks on it it will possibly have two buttons there like what I have here in the image bellow,
and by clicking on the Configure Python Tests it will open a modal with two options one for unittests and other to pytests. I think there could be 3rd option as well Django tests and if user selects this option so there have be a second question, where is the path to main settings.py file of the Django project? and thats what we need! the answer to this question have to be set for DJANGO_SETTINGS_MODULE or the django_settings_module arg.

[mh-firouzjah]

as far as I could go, there is a selectTestRunner function in src/client/testing/configuration/index.ts which will prompts user to select either unittest or pytest so it would be possible to add 3rd option as django test. actually django tests are unittests which means by selecting this option the process is the same as what is for unittest option, but with an extra question and also a function invocation.

• the question is where is the django settings module path?. the answer to this question is used for the DJANGO_SETTINGS_MODULE environment variable.
• the function invocation is to check if django is installed and invoke django.setup().
  as a python script the code is:

import os

def setup_django_test_env(django_settings_module_path)
    try:
        import django
    except ModuleNotFoundError:
        # fail quietly
        return

    os.environ.setdefault("DJANGO_SETTINGS_MODULE", django_settings_module)
    django.setup()

[mh-firouzjah]

Hello, I wanted to provide an update on the progress of the project.
I have created a separate branch (-v2) to work on the TypeScript development, but I have run into some complications and need assistance to complete that portion of the project.
However, I have completed the Python part and anticipate that it will function properly.
I attempted to implement a feature that asks users to select their Django settings module file, but I was unable to complete the GUI portion of this feature.
Nonetheless, using command line variables and a JSON configuration file, I was able to successfully implement this feature.
If possible, could you please review/test the code on the v2 branch?
Also my previous comments was explanations on what I've done in V2 branch.

I am unsure if asking users to enter the Django settings module is the best approach, for a few reasons. Firstly, if there are multiple Django settings modules (for different phases of development/production), it could be overwhelming to constantly change test configurations. Additionally, if configurations are set in the workspace, there is no GUI option to change them, and it would require modifying the JSON file directly.

I believe it may be more beneficial to ask users about the manage.py module instead, in order to read the DJANGO_SETTINGS_MODULE from that file. This would allow for automatic detection of changes to the settings module.

It is still very handy to have the older logic I made. in case of user forgotten to configure django settings module but there are some django tests in the workspace that logic will help to detect the tests and in case of any exception it fails quietly so no harm to have it as a backup system

All that have been said, I think this PR is better than what I prepared in V2 branch because with the changes I have here the Users will never be notified about that your extension will consider unittests and django tests two different kind of tests, which is not True and actually djnago tests are unittests (are subclasses of unittest). and if something goes wrong again it wont notify the user and will fail quietly I like this approach more than the other.

I leave this PR opened and if anybody helps me to finish V2 I will open a new PR for that as well. If any of you could help me with TS that branch will be finished very soon.

Thank you for considering this update, and please let me know if you have any further questions or if I can assist in any way.

[eleanorjboyd]

Thank you for your edits! Will review this soon as I have a few other items I need to get through. Will let you know if any more questions arise!

[mh-firouzjah]

When user is starting testing functionality it could be possible to ask users if they are going to work with django tests like when it prompts to select either unittest or pytest. then by locating manage.py and (optionally settings module) we can setup the environment in such a way that for discovering tests it treats as looking for standard unittests(what is currently implemented cause django tests are subclasses of unittest). But it's better to use manage.py module to execute tests instead, cause this module has specific functionalities for testing purposes. manage.py test command uses flags compatible with unittest library but it also has some extra flags. Important one may be is the positional argument test_label as described:

• positional argument:

  test_label    Module paths to test; can be `modulename`, `modulename.TestCase` or `modulename.TestCase.test_method`

I'm wondering if we could use this manage.py module to visualize the result of the tests execution process, it may be possible to use manage.py test <optional test_labels> instead of what currently has been called for unittests.

[eleanorjboyd]

Thank you for the updates! Will review sometime this week

[mh-firouzjah]

just to confirm that it could work as expected I did a check (of course I didn't generate TS/JS code as it's required for the final release but) passing manage.py module path as a command line argument to visualstudio_py_testlauncher.py worked well, either in absolute or relative path. like the following image.

• You mentioned before that visualstudio_py_testlauncher.py is deprecated and will be removed soon, so I also tried to have the same functionality in execution.py and discovery.py as well so to help the future versions.
• To be clear what happened in the image bellow: I created a django project inside shift-app directory which means inside this directory there was a manage.py module and I had an app called authentication which inside that app I had 8 tests. in terminal I changed directory to inside that app and then by specifying the manage.py module (absolute/relative path) it found and executed all 8 tests. note that normally and for the time being the extension uses visualstudio_py_testlauncher.py to run tests and it can not identify and run django tests at all, even if the workspace is the same as django projects root directory, setting up django env and using the django's built in functionalities to execute tests is done here and worked well. (the actual code at mh-furouzjah-patch-v2)
• Note: that dictionary in the image is not relevant please ignore it.

[eleanorjboyd]

Hi @mh-firouzjah, looking at this again, so sorry for the delay. I have a meeting tomorrow to discuss with a coworker and then should come back with some more questions. Tried this PR out locally and was very impressed with how everything worked together. Excited to move this forward, thanks again!

[eleanorjboyd]

Hello! A few notes.

First is that we are moving to a new design for testing args which will not effect this PR much but will instead have to do with the user experience of running tests. I have just created #22206 which proposes how Django enablement would look in this new setup. Sorry that this extra step is coming into play but we think the environment variables that we can then use with the new arg design will be the best place for these Django settings. I have written up this proposal to get feedback at Django-con so we can get some other perspectives on your design! This should only slightly change the script you wrote to just get these Django enablement variables from env vars instead of args.

On another note, when reviewing this PR one question I did have that I thought you might be able to answer is about teardown. Are these specific steps we need to take at the end of Django testing to teardown databases etc and are those cases being covered via this implementation?

I might add a few edits to this PR but otherwise, it is very great and we would just need to update it for this new arg design. I am more than happy to take this PR from here and see it to finish (obviously giving you credit for your amazing work) if you would like or can continue to involve you in the next steps (like switching to env vars, writing test cases, error handling etc). Let me know whatever you would like and either way, you have been instrumental in getting this on its way!

Huge thank you! This is amazing!

[mh-firouzjah]

On another note, when reviewing this PR one question I did have that I thought you might be able to answer is about teardown. Are these specific steps we need to take at the end of Django testing to teardown databases etc and are those cases being covered via this implementation?

Hello, for the time being the script will uses the normal unittest so it will update database permanently. If any test needs to write/wipe database it will use the database defined inside settings.py` module not a temporary test database.

But django way of running tests which is manage.py test will use a temporary tests-database which obviously is the best solution.

Now because of that, I wan to work on "How to use manage.py test as the actual answer to this feature request".
It is also possible to have a customized version of the functions/classes that the "manage.py test" command uses behind the since. But this will be heavily tied to the current implementation of Django. Even though Django is trying to maintain backwards compatibility as much as possible, there will always be a risk that this approach will run into problems and require constant support for compatibility with newer versions of Django.
The code in V2 branch in pythonFiles/djangotest_adapter/execution.py is a customized version which is inherited from Django and is compatible with vscode unittest discover/runner. I think the best option here is to use manage.py test as the default code of django to both discover and run the tests, but the current implementation of this command in Django does not provide a discover version to just list the test without executing them.

[eleanorjboyd]

What are the advantages of using manage.py for discovery? Are there django tests that would show up / not show up given any setup that is required by manage.py? Wondering if unittest could do the discovery then django could run or if it would miss information like skips or ignores if manage.py didn't handle discovery. I would love to be able to use manage.py directly, compatibility and maintainability is always a priority for us. What do you think would need to be done in order to make this happen? I don't know if you have looked at how pytest works at all (see init.py in vscode-pytest) but if there was a way we could run manage.py then hook into unittest from there that would obviously be ideal.

Would love to hear your thoughts on what is a good MVP vs a long term solution. Is there a way we can add support for Django and get it out soon then consider a more long term approach or is there a long-term solution we can consider now?

Thanks

[eleanorjboyd]

hello! I took into consideration what you said about the ideal solution. I started experimented and came up with #22222. This allows us to run Django tests via manage.py test to provide a more direct solution that allows for database setup / teardown. I am very interested to hear what you have to think and if this could be viable. I am not sure if I did anything against convention (like editing the settings.py) so if that's the case please let me know and what workarounds we could use. Also I am wondering what this would mean for discovery, same question applies above about what Django discovery adds.

[mh-firouzjah]

Wondering if unittest could do the discovery then django could run or if it would miss information like skips or ignores if manage.py didn't handle discovery.

Yes - as long as Django Tests are SubClasses of unittest. If the initial django setup has been done(django.setup() and DJANGO_SETTINGS_MODULE environment variable are set and invoked) unittest could do the discovery. But it's not recommended to use unittest to also execute the tests because as You were right to be concerned, it will not use a temporary database intentionally for tests and will work directly with the production/development database which user would specify(or the default one) in settings.py module.
Fortunately it is possible to use manage.py test to execute tests without having to worry about setting up django environment or preparing a test database and tear it down (because that command handles these things out of the box) but again there are some bottlenecks as manage.py test is a complete 3rd party here, it has nothing to do with the extension, unless finding a way to run this command for example using subprocess modules, to be able to assess the result of it's execution. I mean to see what happened, what tests has been passed successfully which ones failed or ...

• manage.py test does discover the tests but as it's current implementation it has no such a method to pass out the discovered tests without running them. (it's python world so it's possible and easy, to patch it and make this possible)

[eleanorjboyd]

Hello! Do you mind if I close this PR as we think the other one is a more likely plan for the solution?

[mh-firouzjah]

the important thing is to find a solution for the discovered problem, so I close this.