Django Test Runs with Coverage by danila-grobov · Pull Request #24927 · microsoft/vscode-python (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation19 Commits3 Checks46 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
will review- sorry for the delay
Hi @danila-grobov! Could you explain a bit how this PR works I am not sure I totally follow how this will add coverage for Django. From what I see, the PR just skips the Django run path if coverage is enabled (in the environment variable) but I didn't think that would correctly run Django tests if it followed the unittest path. Thanks
Hi,
1. Skipping "django run path"
Most of the changed files in this PR are for testing setup. I used runner_with_cwd_env (a helper function only used in tests) to avoid reinventing the wheel. Other Django tests bypass execution.py, where coverage is set up, so runner_with_cwd_env was designed accordingly. However, my coverage tests required execution.py, so I added a small check to make it work as needed.
2. Implementing Coverage
Coverage wasn’t working because Django tests ran in a subprocess, preventing proper metric collection. To fix this, (in the django_handler.py) I imported the entire Django project module and used manage.py’s main function to run tests within the same process, allowing coverage to track execution correctly.
Hope this helps!
aha! I understand now- fantastic! This is great, thanks so much for putting this together and figuring it out. Especially writing tests to go along with it; I wrote that helper method but haven't looked at it in long enough I forgot about it! This is actually even better because we had faced the issue regarding starting a subprocess as being non-ideal for django run in this issue too: #24242 which means we might be able to solve that as well.
Let me do one final pass through to make sure everything makes sense but otherwise we can get this merged and out on pre-release for people to try it!
Great, glad to hear that we can solve some more issues along the way!
I've tried to keep this PR focused, so I didn't touch the discovery part of the django tests. However, it is also being run in a subprocess. If you take a look at the discovery function in the same file.
Should we change that in this PR too? Would be more consistent. Would change this PR's scope to "Migrating Django tests to a single process" instead of just fixing coverage.
hm good question- lets stick with the PR as is and leave it in pre-release for a few days to make sure we don't get any complaints then we can do the same edit for discovery!
It's my first time contributing, so I'm not too familiar with the process.
Could you describe how does pre-release work? Will we have to merge the PR or does it do it through actions?
Issue identified by VS Code Team member as probable bug
label
@danila-grobov The process is that we merge this PR to main. That allows it to be deployed to pre-release versions of Python extension. People adopting pre-release can use this fix and report any issues with this. We also do our manual testing on the pre-release bits. If things looks good it will get published to stable next month.
Hi sorry for not answering, thanks @karthiknadig for providing that information. @danila-grobov if you have any other questions let me know!
I went ahead and just fixed that pyright issue since I had the branch setup on my machine already so it should now pass those tests and then we can merge it!
Hey, nice work @danila-grobov, I switched to the pre-release version, and now I'm getting this messageError during Django test execution: module 'manage' has no attribute 'main'
What I'm tryin' to do is basically go into a test file and click the "Run Test with Coverage" button, it doesn't really matter which test I run, it's always the same result, let me know if I'm missing something or doing somethin wrong
Received test ids from temp file.
COVERAGE_ENABLED env var set, starting coverage. workspace_root used as parent dir: /home/developer/IdeaProjects/myapp
MANAGE_PY_PATH env var set, running Django test suite.
Django project directory: .
Django manage.py arguments: ['manage.py', 'test', '--testrunner=django_test_runner.CustomExecutionTestRunner', '-p', 'test_*.py', '--keepdb', 'myapp.my_subapp.tests.use_cases.test_file.TestUseCase.test_execute']
Error during Django test execution: module 'manage' has no attribute 'main'
@danila-grobov It's literally this
#!/usr/bin/env python import os import sys
if name == "main": os.environ.setdefault("DJANGO_SETTINGS_MODULE", "myapp.settings") try: from django.core.management import execute_from_command_line except ImportError: # The above import may fail for some other reason. Ensure that the # issue is really that Django is missing to avoid masking other # exceptions on Python 2. try: import django except ImportError: raise ImportError( "Couldn't import Django. Are you sure it's installed and " "available on your PYTHONPATH environment variable? Did you " "forget to activate a virtual environment?" ) raise execute_from_command_line(sys.argv)
I see, Django changed their manage.py template 6 years ago. To include that main function I'm relying on.
I guess I could instead trigger that if statement with "main" so that it would work for older versions too.
@eleanorjboyd @karthiknadig How should I proceed? Do I open a new PR or just reopen this one?
I managed (badum-tss) to fix my issue just placing the entire code inside a main function, It worked like a charm
Love we are getting people trying this feature to give feedback! @danila-grobov let me know if you need help getting in the fix.
unrelated but @danila-grobov I posted on bluesky about my excitement with this PR and might post on X in the future. I just listed you as a "community contributor" because I wasn't sure if you wanted to be mentioned specifically but let me know if you have an account on either you want me to tag! Thanks
Oh wow, I feel very honored! ♥️
You can just mention me by name I guess, unfortunately I don't have an account on either.
karthiknadig pushed a commit that referenced this pull request
Related to this issue: #24199
@mcobalchinisoftfocus Discovered an issue with older django versions, which didn't have the main function in the manage.py
I've fixed this issue by executing the code in manage.py with name set to main instead of relying on main function being there.
I've also adjusted the test, so that it would cover this case.
Labels
Issue identified by VS Code Team member as probable bug