Consistently pass verbosity flag to VCS module

[ByteB4rb1e]

Implements #13329

As suggested by @uranusjr, I've adopted the interface of the already existing cascade for verbosity flagging.

Even though my feature request is only applicable to the Git VCS module, the interface of the super class forced me to also implement the verbosity flagging for all other subclasses.

Since the feature is applicable to subprocesses calls, which only causes changes to their stdout, I did not deem it feasible to write specific unit tests for this feature.

[ByteB4rb1e]

I'm currently trying to isolate the failing test-cases locally. I'm executing nox in a venv but haven't yet gotten to the point on how to pass extra arguments to the test session's pytest invocation.

Will resume in the next couple of days, first checking the expected output of the test cases, then figuring out what my implementation has changed. I doubt/hope it's anything major.

[ichard26]

@ByteB4rb1e You can pass extra arguments to the pytest invocation by adding -- followed by the extra arguments. For example:

nox -s test-3.12 -- tests/functional --collect-only

This is also covered in the pip contributing documentation: https://pip.pypa.io/en/stable/development/getting-started/#running-tests

[ByteB4rb1e]

One more thing that came to mind as I couldn't find a definitive requirement in the Development - Getting Started guide: Should the news fragment reference the issue the PR resolves, or the PR itself? I currently am referencing the issue it resolves.

[uranusjr]

Would it be a good idea if we add some mock-based tests so we are more confident what args are passed to the underlying module in what cases?

[ByteB4rb1e]

Would it be a good idea if we add some mock-based tests so we are more confident what args are passed to the underlying module in what cases?

@uranusjr yes, definetly! Testing the interface at least makes sense...

EDIT: reformatted and was able to identify the causes for 1. and 2., was related to how I interacted with the test runner.

I still have to reformat the test cases, but ran into two seperate issues:

1. I'm unable to get the result I'm expecting when calling the src/pip/_internal/vcs/git.py/Git::update_submodules method from within any of the shared super class methods (update, fetch_new). - See 5b863da, for more information. I will have to further investigate as to why...

2. For some reason I'm unable to get the method signatures to update when running pytest. I'm receiving TypeErrors saying that verbosity is an unexpected keyword argument. The arguments are defined for all applicable methods, yet when I debug with print(help(self.svn)) within the test cases, I can see that the instances don't have the argument defined...

[ByteB4rb1e]

The test case executions look better than I expected. All of this is redeemable. Though I can't debug locally with my current setup, as the results look vastly different... And I wasn't able to execute the failing tests properly yet. I don't quite understand why pytest executes differently on my local machine, regardless of using nox as a wrapper, or not... Is there some pytest plugin that I'm maybe unaware of which checks out a specific Git revision?

EDIT: Didn't understand the setup of the testing framework, do now... With nox bootstrapping it's working, albeit slow. The -R flag as defined in noxfile.py helps, might open an issue whether it would be practical to document this in the development guide?

$> sh -x << 'EOF'
cd "$(mktemp -d)"

git clone --depth 1 --single-branch --branch feature/13329 \
    https://github.com/ByteB4rb1e/pip.git .

python3 -m venv .venv

.venv/bin/python3 -m pip install --quiet --upgrade \
    -r tests/requirements.txt -r tests/requirements-common_wheels.txt \
    pip pytest nox

.venv/bin/pytest tests/unit/test_vcs.py::TestBazaarArgs
EOF
+ mktemp -d
+ cd /tmp/tmp.MioIic61HV
+ git clone --depth 1 --single-branch --branch feature/13329 https://github.com/ByteB4rb1e/pip.git .
Cloning into '.'...
remote: Enumerating objects: 1087, done.
remote: Counting objects: 100% (1087/1087), done.
remote: Compressing objects: 100% (959/959), done.
remote: Total 1087 (delta 71), reused 550 (delta 54), pack-reused 0 (from 0)
Receiving objects: 100% (1087/1087), 8.86 MiB | 825.00 KiB/s, done.
Resolving deltas: 100% (71/71), done.
+ python3 -m venv .venv
+ .venv/bin/python3 -m pip install --quiet --upgrade -r tests/requirements.txt -r tests/requirements-common_wheels.txt pip pytest
+ .venv/bin/pytest tests/unit/test_vcs.py::TestBazaarArgs
=============================== test session starts ================================
self = <tests.unit.test_vcs.TestBazaarArgs testMethod=test_update_quiet>

    def test_update_quiet(self) -> None:
>       self.svn.update(self.dest, hide_url(self.url), self.rev_options, verbosity=0)
E       TypeError: Bazaar.update() got an unexpected keyword argument 'verbosity'

tests/unit/test_vcs.py:831: TypeError
============================= short test summary info ==============================
FAILED tests/unit/test_vcs.py::TestBazaarArgs::test_update - TypeError: Bazaar.update() got an unexpected keyword argument 'verbosity'
FAILED tests/unit/test_vcs.py::TestBazaarArgs::test_update_quiet - TypeError: Bazaar.update() got an unexpected keyword argument 'verbosity'
=========================== 2 failed, 3 passed in 0.16s ============================

[ichard26]

@ByteB4rb1e honestly I'm not sure I understand your terminal session, but you don't have to install the test requirements manually in your development environment. Nox will create and manage the testing environment for you. By default, we have nox configured to reinstall the environment dependencies. It's generally fast as the underlying environment is kept between runs, but it does still add some overhead (especially from the reinstall of the local pip). The -R flag can be used if you'd like to retest the same code with a different set of pytest arguments as you've discovered.

I didn't add the -R flag to the pip contributing documentation mostly because it was intended as a CI optimization, but I can see how it can be helpful. Ironically enough, I took another at look at my PR and I did say that the -R / --no-install flag would be more helpful for local development #13126 (comment). Oops 🙂

It would be great if you could document this in the development guide!¹

Footnotes

1. I don't even use Nox for the most part. I run pytest directly for day to day development as it is (or was before -R) faster than using Nox. ↩

[ichard26]

Thanks for the contribution by the way!

[ByteB4rb1e]

@ByteB4rb1e honestly I'm not sure I understand your terminal session, but you don't have to install the test requirements manually in your development environment.

@ichard26, my development environment is MSYS2, which emulates a POSIX environment under Windows. There are a couple of caveats to it, especially in regards to filesystem performance, hence I tend to avoid IO-intensive workflow abstractions in favor of a manual workflow. This translates to basically the same approach you referenced in the footnote. For some reason though executing pytest directly against the pip sources resulted in some strange behavior of the test runner somehow testing against old code. I've cleared the caches, but that didn't help. The pragmatic solution was to just use nox, as I couldn't reproduce the behavior there. The overhead of executing a single test case though was so immense, I thankfully stumbled upon your addition in the noxfile.py, which eased the problem quite a bit!

I'm not sure if it's worthy of an issue report, but I'd really be interested if somebody was able to reproduce the behavior I described in regards to test executions without nox. I removed the last line of the script executing nox (i've posted in an earlier comment), as this was just to illustrate the deviating behavior. The script should be idempotent. I can keep my fork live, if an issue report makes sense.

It would be great if you could document this in the development guide

Sure, I'll open a feature request later on.