Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Django Test Runs with Coverage#24927

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
karthiknadig merged 3 commits intomicrosoft:mainfromdanila-grobov:django_coverage
Apr 3, 2025

Conversation

danila-grobov
Copy link

@danila-grobovdanila-grobov commentedMar 22, 2025
edited
Loading

@eleanorjboyd
Copy link
Member

will review- sorry for the delay

@eleanorjboyd
Copy link
Member

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

@danila-grobov
Copy link
Author

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!

@eleanorjboyd
Copy link
Member

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!

@danila-grobov
Copy link
Author

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.

@eleanorjboyd
Copy link
Member

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!

danila-grobov reacted with thumbs up emoji

@danila-grobov
Copy link
Author

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?

@karthiknadigkarthiknadig added the bugIssue identified by VS Code Team member as probable bug labelApr 3, 2025
karthiknadig
karthiknadig previously approved these changesApr 3, 2025
@vs-code-engineeringvs-code-engineeringbot added this to theApril 2025 milestoneApr 3, 2025
@karthiknadigkarthiknadigenabled auto-merge (squash)April 3, 2025 21:23
@karthiknadig
Copy link
Member

@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.

@karthiknadig
Copy link
Member

@eleanorjboyd
Copy link
Member

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!

danila-grobov reacted with heart emoji

eleanorjboyd
eleanorjboyd previously approved these changesApr 3, 2025
@karthiknadigkarthiknadig merged commite9fb2bf intomicrosoft:mainApr 3, 2025
46 checks passed
@mcobalchinisoftfocus

Hey, nice work@danila-grobov, I switched to the pre-release version, and now I'm getting this message
Error 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/myappMANAGE_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
Copy link
Author

@mcobalchinisoftfocus Hmm that's weird. How does your manage.py look like? Does it have a main function?

@mcobalchinisoftfocus

@danila-grobov It's literally this

#!/usr/bin/env pythonimportosimportsysif__name__=="__main__":os.environ.setdefault("DJANGO_SETTINGS_MODULE","myapp.settings")try:fromdjango.core.managementimportexecute_from_command_lineexceptImportError:# 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:importdjangoexceptImportError:raiseImportError("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?"            )raiseexecute_from_command_line(sys.argv)

@danila-grobov
Copy link
Author

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?

eleanorjboyd reacted with heart emoji

@karthiknadig
Copy link
Member

@danila-grobov Create a new PR with the fix.

danila-grobov and eleanorjboyd reacted with thumbs up emoji

@mcobalchinisoftfocus

I managed (badum-tss) to fix my issue just placing the entire code inside a main function, It worked like a charm

@eleanorjboyd
Copy link
Member

Love we are getting people trying this feature to give feedback!@danila-grobov let me know if you need help getting in the fix.

@eleanorjboyd
Copy link
Member

unrelated but@danila-grobov Iposted 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

mcobalchinisoftfocus reacted with heart emoji

@danila-grobov
Copy link
Author

Oh wow, I feel very honored!♥️

You can just mention me by name I guess, unfortunately I don't have an account on either.

eleanorjboyd and mcobalchinisoftfocus reacted with heart emoji

karthiknadig pushed a commit that referenced this pull requestApr 7, 2025
Related to this issue:#24199@mcobalchinisoftfocus Discovered an issue with older django versions,which didn't have the main function in the manage.py#24927 (comment)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.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@karthiknadigkarthiknadigkarthiknadig approved these changes

@eleanorjboydeleanorjboydeleanorjboyd approved these changes

Assignees

@eleanorjboydeleanorjboyd

Labels
bugIssue identified by VS Code Team member as probable bug
Projects
None yet
Milestone
April 2025
Development

Successfully merging this pull request may close these issues.

5 participants
@danila-grobov@eleanorjboyd@karthiknadig@mcobalchinisoftfocus@DanilaGrobovSEB

[8]ページ先頭

©2009-2025 Movatter.jp