Switch to pytest and tox #1763

saroad2 · 2020-10-14T12:41:29Z

It became a good practice in python to use Tox. In that way we can make sure that the contributors of black test in the same environment as the CI process.

Moreover, I propose moving to use Pytest as the testing infrastructure because it could make our tests much clearer. Consider the following tests from test_black.py:

    ...
    @patch("black.dump_to_file", dump_to_stderr)
    def test_function(self) -> None:
        source, expected = read_data("function")
        actual = fs(source)
        self.assertFormatEqual(expected, actual)
        black.assert_equivalent(source, actual)
        black.assert_stable(source, actual, DEFAULT_MODE)

    @patch("black.dump_to_file", dump_to_stderr)
    def test_function2(self) -> None:
        source, expected = read_data("function2")
        actual = fs(source)
        self.assertFormatEqual(expected, actual)
        black.assert_equivalent(source, actual)
        black.assert_stable(source, actual, DEFAULT_MODE)
    ...

Those are practically the same test. Why do we need to write it twice? with pytest we could simply write:

    ...
    @patch("black.dump_to_file", dump_to_stderr)
    @pytest.mark.parametrize("file_name", ["function", "function2"])
    def test_function(self, file_name) -> None:
        source, expected = read_data(file_name)
        actual = fs(source)
        self.assertFormatEqual(expected, actual)
        black.assert_equivalent(source, actual)
        black.assert_stable(source, actual, DEFAULT_MODE)
    ...

I think both changes will be beneficial for Black.

What do you think?

cooperlees

Thanks for this. I'm all for making our tests runnable from tox and pytest, but not enforcing the use of pytest in our CI. What I'm not a big fan of is pytest's magic and overloading of standard python functionality (e.g. assert) and that it's now another dependency to black to test. But I'll live with it if other black core devs like pytest, I've had to learn it and deal with it for other projects.

Aside from this, love the refactor and split of blackd tests. Love the clearer deps now too.

cooperlees · 2020-10-14T14:38:37Z

.github/workflows/fuzz.yml

          python -m pip install --upgrade pip
-          python -m pip install --upgrade coverage
-          python -m pip install --upgrade hypothesmith
-          python -m pip install -e ".[d]"
+          python -m pip install --upgrade tox


Why not install together?
python -m pip install --upgrade pip tox

Maybe the newest pip has something new and exciting, and we want to use the new and exciting pip for the other stuff?

Just seems a waste of an extra fork/exec for now ... If we hit a bug then I'd change back to the two executes ... Not a big deal, thus why I didn't request changes ...

I'm with @hugovk on this.

If it were a single call to pip, then there's no point upgrading pip (unless it is cached for next time, and it's not).

A concrete reason:

For Python 3.8 on Windows, pip 19.3+ is needed to install wheels (at least for Pillow). We've added the double command to the installation instructions, and that came up again this week.

This isn't specifically needed for Black test right now, but I suggest skipping the pip upgrade altogether and using the CI image's version, or having two calls.

(Right now, pip in the image is the latest version, so the call is really quick; and it's pretty quick for an upgrade as well.)

cooperlees · 2020-10-14T14:38:55Z

.github/workflows/test.yml

          python -m pip install --upgrade pip
-          python -m pip install --upgrade coverage
-          python -m pip install -e ".[d]"
+          python -m pip install --upgrade tox


Same - Install both?

saroad2 · 2020-10-14T14:56:36Z

I'm not in love with the idea of Pytest, but I have a great experience with it in all of my repositories.

If I cannot use pytest.mark.parametrize, may I suggest using this instead. Never tried it so I don't know how it is, but I think it worths a try. Maybe I can try to move tests to use it in another PR after you merge this one.

cooperlees · 2020-10-14T15:02:34Z

I'm not in love with the idea of Pytest, but I have a great experience with it in all of my repositories.

If I'm cannot use pytest.mark.parametrize, may I suggest using this instead. Never tried it so I don't know how it is, but I think it worths a try. Maybe I can try to move tests to use it in another PR after you merge this one.

Yes, let's definitely do the test conversions in a separate PR.

Also, lets wait and get other Core Devs for it before doing the conversion. Support is one thing, going pytest only is another.

I just want to state I'm not blocking pytest, but sharing I'm not for it. Mainly due to how much long, due to it's magic it's taken me to debug things. But that's primarily cause I don't use it primarily.

ichard26 · 2020-10-18T00:41:00Z

While on the topic of In that way we can make sure that the contributors of black test in the same environment as the CI process., perhaps instead of doing a regular pip editable install, a pipenv development install should be done. This makes sure that the CI is using the identical modules and packages that developers of Black use and therefore increasing reproducibility . The other benefits I can think of include:

Good caching can be implemented allowing feedback to contributors be to returned faster
When a dependency upgrade happens, the change is automatically tested. This makes using Dependabot or other services safer to use. It also protects us from *me* messing up a dependency upgrade like in PR Fix toml parsing and bump toml from 0.10.0 to 0.10.1 #1501 and then PR Fix toml hashes and make it clear that only TOML is supported #1510 :-)

The main downside is that Black incompatibilities with newer versions of modules / packages won't be automatically discovered since we'll be pinning to probably older versions. Using PyUp or other services mostly mitigates this issue though.

Just some ideas to think about.

cooperlees · 2020-10-18T21:22:40Z

While on the topic of In that way we can make sure that the contributors of black test in the same environment as the CI process., perhaps instead of doing a regular pip editable install, a pipenv development install should be done. This makes sure that the CI is using the identical modules and packages that developers of Black use and therefore increasing reproducibility . The other benefits I can think of include:

Good caching can be implemented allowing feedback to contributors be to returned faster

When a dependency upgrade happens, the change is automatically tested. This makes using Dependabot or other services safer to use. It also protects us from me messing up a dependency upgrade like in PR Fix toml parsing and bump toml from 0.10.0 to 0.10.1 #1501 and then PR Fix toml hashes and make it clear that only TOML is supported #1510 :-)

The main downside is that Black incompatibilities with newer versions of modules / packages won't be automatically discovered since we'll be pinning to probably older versions. Using PyUp or other services mostly mitigates this issue though.

Just some ideas to think about.

If you do the PRs I'm sure we'll merge them :) ...

I love PyUp method, but I always like to have both a pinned and unpinned options. This is due to having situations where I've had to fork to unpin to install things in some environments. And I've not wanted to inflict that pain on users of libraries I've maintained.

cooperlees

This is an improvement to allow for Devs to get a CI like env easier. Lets merge it.

cooperlees · 2020-10-19T17:35:22Z

No one has spoke up about not liking this. I'm merging.

jayaddison · 2021-02-15T19:13:59Z

tox.ini

+    -r{toxinidir}/test_requirements.txt
+    hypothesmith
+    lark-parser < 0.10.0
+; lark-parser's version is set due to a bug in hypothesis. Once it solved, that would be fixed.


@saroad2 It looks like lark-parser's been updated in the test dependencies for hypothesmith - could/should we remove this version pin?

I guess we should :) I added this pinning so the tests would pass.

* Add venv to .gitignore * Use tox to run tests * Make fuzz run in tox * Split tests files * Fix import error

ichard26 · 2021-09-18T17:11:57Z

Hi @saroad2, thanks for moving us to pytest, it's been a better experience personally. Anyway I'm working on removing parameterized as a test suite dependency and I noticed pytest-mock + pytest-cases. I'm not familiar with these plugins and AFAICT the test suite doesn't use these plugins ATM. If that's the case I'd wish to remove them. Why were they added?

Thank you in advance!

saroad2 · 2021-09-21T10:01:22Z

Hey @ichard26, I think I added them by mistake.

However, if you take a close look at pytest-cases, I think you'll find it as a very helpful plugin to pytest.
Using it might cause you to rewrite some tests, but in a much prettier form, in my humble opnion.

saroad2 added 5 commits October 14, 2020 15:40

Add venv to .gitignore

8f29b1f

Use tox to run tests

b3e07b4

Make fuzz run in tox

3a47610

Split tests files

23e46e0

Fix import error

f21e761

cooperlees reviewed Oct 14, 2020

View reviewed changes

cooperlees approved these changes Oct 18, 2020

View reviewed changes

cooperlees merged commit 4070527 into psf:master Oct 19, 2020

saroad2 deleted the switch_to_pytest_and_tox branch October 19, 2020 18:11

jayaddison reviewed Feb 15, 2021

View reviewed changes

noxan pushed a commit to noxan/black that referenced this pull request Jun 6, 2021

Switch to pytest and tox (psf#1763)

0cf5dda

* Add venv to .gitignore * Use tox to run tests * Make fuzz run in tox * Split tests files * Fix import error

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to pytest and tox #1763

Switch to pytest and tox #1763

saroad2 commented Oct 14, 2020 •

edited

Loading

cooperlees left a comment •

edited

Loading

cooperlees Oct 14, 2020

hugovk Oct 14, 2020

cooperlees Oct 14, 2020 •

edited

Loading

saroad2 Oct 14, 2020

hugovk Oct 14, 2020

cooperlees Oct 14, 2020

saroad2 commented Oct 14, 2020 •

edited

Loading

cooperlees commented Oct 14, 2020

ichard26 commented Oct 18, 2020

cooperlees commented Oct 18, 2020

cooperlees left a comment

cooperlees commented Oct 19, 2020

jayaddison Feb 15, 2021

saroad2 Feb 15, 2021

ichard26 commented Sep 18, 2021

saroad2 commented Sep 21, 2021

Switch to pytest and tox #1763

Switch to pytest and tox #1763

Conversation

saroad2 commented Oct 14, 2020 • edited Loading

cooperlees left a comment • edited Loading

Choose a reason for hiding this comment

cooperlees Oct 14, 2020

Choose a reason for hiding this comment

hugovk Oct 14, 2020

Choose a reason for hiding this comment

cooperlees Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

saroad2 Oct 14, 2020

Choose a reason for hiding this comment

hugovk Oct 14, 2020

Choose a reason for hiding this comment

cooperlees Oct 14, 2020

Choose a reason for hiding this comment

saroad2 commented Oct 14, 2020 • edited Loading

cooperlees commented Oct 14, 2020

ichard26 commented Oct 18, 2020

cooperlees commented Oct 18, 2020

cooperlees left a comment

Choose a reason for hiding this comment

cooperlees commented Oct 19, 2020

jayaddison Feb 15, 2021

Choose a reason for hiding this comment

saroad2 Feb 15, 2021

Choose a reason for hiding this comment

ichard26 commented Sep 18, 2021

saroad2 commented Sep 21, 2021

saroad2 commented Oct 14, 2020 •

edited

Loading

cooperlees left a comment •

edited

Loading

cooperlees Oct 14, 2020 •

edited

Loading

saroad2 commented Oct 14, 2020 •

edited

Loading