Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Django Test Compatibility #23935

Merged
merged 17 commits into from
Aug 14, 2024
Merged

Django Test Compatibility #23935

merged 17 commits into from
Aug 14, 2024

Conversation

eleanorjboyd
Copy link
Member

implements and thus closes #22206
resolves #73!

@eleanorjboyd eleanorjboyd added the feature-request Request for new features or functionality label Aug 9, 2024
@eleanorjboyd eleanorjboyd self-assigned this Aug 9, 2024
@eleanorjboyd eleanorjboyd marked this pull request as ready for review August 13, 2024 15:08
@vs-code-engineering vs-code-engineering bot added this to the August 2024 milestone Aug 13, 2024
python_files/tests/pytestadapter/helpers.py Outdated Show resolved Hide resolved
python_files/tests/unittestadapter/test_discovery.py Outdated Show resolved Hide resolved
python_files/tests/unittestadapter/test_discovery.py Outdated Show resolved Hide resolved
python_files/tests/unittestadapter/test_execution.py Outdated Show resolved Hide resolved
python_files/tests/unittestadapter/test_execution.py Outdated Show resolved Hide resolved
python_files/unittestadapter/discovery.py Outdated Show resolved Hide resolved
python_files/unittestadapter/execution.py Outdated Show resolved Hide resolved
python_files/unittestadapter/execution.py Outdated Show resolved Hide resolved
Comment on lines +46 to +53
subprocess_discovery = subprocess.run(
command,
capture_output=True,
text=True,
env=env,
)
print(subprocess_discovery.stderr, file=sys.stderr)
print(subprocess_discovery.stdout, file=sys.stdout)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be able to stream the output from the subprocess. The way it is right now, the output will not be seen this later. This could be handled in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet ya let me add a new issue to do this in a separate PR

Comment on lines +93 to +100
subprocess_execution = subprocess.run(
command,
capture_output=True,
text=True,
env=env,
)
print(subprocess_execution.stderr, file=sys.stderr)
print(subprocess_execution.stdout, file=sys.stdout)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, output from subprocess can be streamed.

@eleanorjboyd eleanorjboyd merged commit 59a8d03 into microsoft:main Aug 14, 2024
40 checks passed
@eleanorjboyd eleanorjboyd deleted the django-2 branch August 14, 2024 20:04
@khamaileon
Copy link

@eleanorjboyd thank you very much for this work, which the Django community has been awaiting for years. Is it already released? Is there a documentation on how to use it?

@eleanorjboyd
Copy link
Member Author

Yes happy to help and appreciate the community support! Here are the instructions: #73 (comment)

It is released on the pre-release version of the extension so you can access it and give it a try! I would appreciate if you are able to try it and let me know how it goes or any bugs! Thanks

@msmitherdc
Copy link

@eleanorjboyd I'm getting this error on Django 5, looks like the verbosity param is missing a level in the command

2024-08-15 18:47:00.679 [info] MANAGE_PY_PATH is set, running Django discovery with path to manage.py as: $/home/gridusr/GRiD/manage.py

2024-08-15 18:47:00.680 [info] Running Django tests with command: ['/home/gridusr/GRiD/virtualenv/production/bin/python', '/home/gridusr/GRiD/manage.py', 'test', '--testrunner=django_test_runner.CustomDiscoveryTestRunner', '-v', '-s', '.', '-p', '*test.py']

usage: manage.py test [-h] [--noinput] [--failfast] [--testrunner TESTRUNNER]
                      [-t TOP_LEVEL] [-p PATTERN] [--keepdb]
                      [--shuffle [SEED]] [-r] [--debug-mode] [-d]
                      [--parallel [N]] [--tag TAGS]
                      [--exclude-tag EXCLUDE_TAGS] [--pdb] [-b]
                      [--no-faulthandler] [--timing] [-k TEST_NAME_PATTERNS]
                      [--durations N] [--version] [-v {0,1,2,3}]
                      [--settings SETTINGS] [--pythonpath PYTHONPATH]
                      [--traceback] [--no-color] [--force-color]
                      [test_label ...]
manage.py test: error: argument -v/--verbosity: expected one argument

@msmitherdc
Copy link

After removing the -v and -s from the generated args, discover works. When running tests, I get

 /home/gridusr/GRiD/virtualenv/production/bin/python /home/gridusr/GRiD/manage.py test --testrunner=django_test_runner.CustomExecutionTestRunner apps/ -p test*.py access.tests.test_any.TestViews.test_user_set_modal

ModuleNotFoundError: No module named 'django_test_runner'

@eleanorjboyd
Copy link
Member Author

Hi @msmitherdc thank you so much for trying this! Glad discovery worked and you got the arg issue figured out- I will need to update my instructions to reflect that.

Secondly for the module not found error- very interesting. If you see in this file, both discovery and execution are called the same way with the addition to the path variable and then calling the subprocess with that custom runner. https://github.com/microsoft/vscode-python/blob/main/python_files/unittestadapter/django_handler.py.

What type of computer are you on? Also what Django and python versions are you on?

My best guess is that these lines are the issue https://github.com/microsoft/vscode-python/blob/main/python_files/unittestadapter/django_handler.py#L69C4-L79C56 and that somehow the PYTHONPATH is not being built correctly even though the steps are the same as for discovery. We might be hitting the first flow in the if statement instead of the second and the pathsep isn't working. Let me investigate a bit more and ill get back to you- thanks!

@msmitherdc
Copy link

For the computer and django type, UI VSCode is Mac ARM64 calling over remote-tunnel to docker connection to container app. Django is 5.0.8 with python 3.12

@eleanorjboyd
Copy link
Member Author

@msmitherdc are you able to try it without the remote connection? Sometimes this can add a layer of complexity especially when we are spinning up subprocesses.

@mbidewell
Copy link

mbidewell commented Aug 18, 2024

Got it fired up today. A little tricky to setup but great as a first step. Thank you so much for your work on this. One thing I noticed is that, in our case, the Django apps are in a subdirectory. I cam make things work by using the manage.py toplevel option, however, the "open unit test" button on the discovery screen cannot find files.

Update, setting the CWD for tests seems to help

@eleanorjboyd
Copy link
Member Author

hi @mbidewell, excited to hear you got it up and working! The setup is definitely challenging now and we plan to make improvements so we will look to gather community feedback then.

Glad you got it working and happy coding!

@mbidewell
Copy link

hi @mbidewell, excited to hear you got it up and working! The setup is definitely challenging now and we plan to make improvements so we will look to gather community feedback then.

Glad you got it working and happy coding!

It's a great step forward. One thing I noticed though. I work on 3 projects. 2 seem to work correctly. 1 has a large number of tests. That project never returns when tests are run. Are there currently any known limits?

@eleanorjboyd
Copy link
Member Author

@mbidewell What do you mean by never returns? And how many tests are you talking about? Only know limitation right now is its incompatible with custom django runners but otherwise no others have been surfaced. This issue you are facing seems like a bug- would you be able to create an issue for it and we can talk / track it there?

@mbidewell
Copy link

mbidewell commented Aug 21, 2024

@mbidewell What do you mean by never returns? And how many tests are you talking about? Only know limitation right now is its incompatible with custom django runners but otherwise no others have been surfaced. This issue you are facing seems like a bug- would you be able to create an issue for it and we can talk / track it there?

After some more experimentation I found the issue. If a test run is aborted uncleanly it can leave behind a test database that manage.py will prompt you to remove on the next run. The text for the prompt was being swallowed and manage.py was blocking waiting for a response. Once I manually removed the test DB everything worked. I added --noinput to the args to hopefully prevent the issue. The swallowing of output might be related to:
#23949

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposed design for Django testing on rewrite Feature suggestion: run Django unittests
6 participants