1357 convert unittest to pytest #3931

ggurjar333 · 2024-10-26T19:29:24Z

Overview

Closes #1357.

What problem does this address?

Replacing Unittest with Pytest to standardizing the testing code.

What did you change?

I have changed Unittest based code with Pytest

Documentation

Make sure to update relevant aspects of the documentation.

Tasks

Give feedback

Update the release notes: reference the PR and related issues.
Update relevant Data Source jinja templates (see docs/data_sources/templates).
Update relevant table or source description metadata (see src/metadata).
Review and update any other aspects of the documentation that might be affected by this PR.
Options

Testing

How did you make sure this worked? How can a reviewer verify this?

To-do list

Give feedback

If updating analyses or data processing functions: make sure to update or write data validation tests (e.g. test_minmax_rows()).
Run make pytest-coverage locally to ensure that the merge queue will accept your PR.
Review the PR yourself and call out any questions or issues you have.
For minor ETL changes or data additions, once make pytest-coverage passes, make sure you have a fresh full PUDL DB downloaded locally, materialize new/changed assets and all their downstream assets and run relevant data validation tests using pytest and --live-dbs.
For bigger ETL or data changes run the full ETL locally and then run the data validations using make pytest-validate.
Alternatively, run the build-deploy-pudl GitHub Action manually.
Options

…o-pytest

…ggurjar333/pudl into 1357-convert-unittest-to-pytest

For more information, see https://pre-commit.ci

…ggurjar333/pudl into 1357-convert-unittest-to-pytest

For more information, see https://pre-commit.ci

…ggurjar333/pudl into 1357-convert-unittest-to-pytest

zaneselvans · 2024-10-31T16:52:34Z

@ggurjar333 I think you'll want to install the git pre-commit hooks locally so that all the static code analysis and formatting checks can be run before anything gets pushed. Some pointers in our docs here

ggurjar333 · 2024-11-01T12:46:17Z

@ggurjar333 I think you'll want to install the git pre-commit hooks locally so that all the static code analysis and formatting checks can be run before anything gets pushed. Some pointers in our docs here

@zaneselvans
I'm facing these errors.

I have installed pre-commit

I think it is messing around here somewhere.

I tried to delete cache this way as well.

How to fix this one?

zaneselvans · 2024-11-01T14:49:21Z

It looks like there are a bunch of genuine test failures on your branch, so I think we should try and separate the errors that are due to changes from the errors that are due to environment / setup issues.

Were you able to successfully run the unit tests before making any changes?
Check out the main branch and run make pytest-unit from the top-level of the repository.
What kind of errors come up then?
How did you create the pudl-dev conda environment?

ggurjar333 · 2024-11-02T06:32:45Z

It looks like there are a bunch of genuine test failures on your branch, so I think we should try and separate the errors that are due to changes from the errors that are due to environment / setup issues.

Were you able to successfully run the unit tests before making any changes?

Check out the main branch and run make pytest-unit from the top-level of the repository.

What kind of errors come up then?

How did you create the pudl-dev conda environment?

Explanation of the issue for the record here:
I am using WSL on Windows. The issue was I had mounted a volume from the Linux side, the disconnect may be what those two Windows vs Linux file paths difference, even if they were trying to actually pointing at the same files on disk. I was trying to commit from within PyCharm folder from linux (as the folder was on Windows) or some shell thing that PyCharm does it might see the Windows path, while pre-commit sees the Linux path.

Solution:
Zane helped me to understand and I moved the folder from Windows path to Linux path at the root directory. It Works!

jdangerx

Overall this looks much nicer when we're consistently using pytest - thank you! I do think that there are some spots where we're using the legacy setup / teardown behavior - while we're here converting everything to the new style, I think we should finish the job and convert those into pytest fixtures.

jdangerx · 2024-11-08T21:44:59Z

test/unit/extract/csv_test.py

@@ -16,16 +12,16 @@


 class FakeExtractor(CsvExtractor):
-    def __init__(self):
+    def __init__(self, mocker):


Non-blocking issue - but I think the FakeExtractor should expect to be passed a ds and not the Pytest mocker fixture. No reason the FakeExtractor needs to know what class the datastore it's being passed is.

jdangerx · 2024-11-08T21:57:40Z

test/unit/extract/csv_test.py

-        ),
-    ):
-        res = extractor.extract(**PARTITION)
+    mocker.patch.object(CsvExtractor, "load_source", return_value=df)


What was the reasoning here for pulling these mocker.patchs out of a context manager?

jdangerx · 2024-11-08T21:59:26Z

test/unit/extract/csv_test.py

    assert len(res) == 1  # Assert only one page extracted
    assert list(res.keys()) == [PAGE]  # Assert it is named correctly
    assert (
        res[PAGE][company_field][0] == company_data
    )  # Assert that column correctly renamed and data is there.


-@patch.object(FakeExtractor, "METADATA")
-def test_validate_exact_columns(mock_metadata, extractor):
+def test_validate_exact_columns(mocker, extractor):


Cool! Thanks for cleaning up the @patch.object(FakeExtractor, "METADATA").

jdangerx · 2024-11-08T22:04:13Z

test/unit/extract/excel_test.py

    """Tests basic operation of the excel.Metadata object."""

+    @pytest.fixture(autouse=True)


This works, but the recommended way of implementing these xunit-style setup methods is to use def setup_class: https://docs.pytest.org/en/7.1.x/how-to/xunit_setup.html#class-level-setup-teardown

But, the even more recommended thing is to just turn this into a normal fixture called metadata and refer to that everywhere we need it.

jdangerx · 2024-11-08T22:05:33Z

test/unit/extract/phmsagas_test.py

@@ -8,20 +6,20 @@


 class FakeExtractor(Extractor):
-    def __init__(self):
+    def __init__(self, mocker):


See above non-blocking comment about passing in the actual dependencies vs. passing in mocker.

jdangerx · 2024-11-08T22:12:38Z

test/unit/output/ferc1_test.py

@@ -185,6 +189,7 @@ def setUp(self):
        )

    def test_leafward_prop_undecided_children(self):
+        """Test leadward propagation with undecided children."""


Were pre-commit checks yelling at you for not adding docstrings to these test methods? If so we should tell the check to ignore test docstrings.

jdangerx · 2024-11-08T22:12:59Z

test/unit/output/ferc1_test.py

@@ -185,6 +189,7 @@ def setUp(self):
        )

    def test_leafward_prop_undecided_children(self):
+        """Test leadward propagation with undecided children."""


Suggested change

"""Test leadward propagation with undecided children."""

"""Test leafward propagation with undecided children."""

jdangerx · 2024-11-08T22:15:09Z

test/unit/workspace/datastore_test.py

@@ -243,7 +209,8 @@ class TestZenodoFetcher(unittest.TestCase):
        r"^10\.(5072|5281)/zenodo\.(\d+)$", PROD_EPACEMS_DOI
    ).group(2)

-    def setUp(self):
+    @pytest.fixture(autouse=True)
+    def setup(self):


Another flag for turning this into normal pytest fixtures.

jdangerx · 2024-11-08T22:19:29Z

test/unit/workspace/resource_cache_test.py

    """Unit tests for the LocalFileCache class."""

-    def setUp(self):
+    def setup_method(self):


I think this setup/teardown should be turned into a fixture as well.

jdangerx · 2024-11-08T22:19:52Z

test/unit/workspace/resource_cache_test.py

    """Unit tests for LayeredCache class."""

-    def setUp(self):
+    def setup_method(self):


Flagging another spot to fixture-ify!

ggurjar333 and others added 20 commits October 19, 2024 21:03

Refactor CsvExtractor unit tests for better test independence

e5d27fe

Refactor unit tests for better test independence

426bc7f

Refactor unit tests for better test independence

f27df16

Merge branch 'catalyst-cooperative:main' into 1357-convert-unittest-t…

f72296c

…o-pytest

Refactor unit tests for better test independence

babf79a

Merge branch '1357-convert-unittest-to-pytest' of https://github.com/…

7eebc13

…ggurjar333/pudl into 1357-convert-unittest-to-pytest

Refactor unit tests for better test independence

90ed32c

Remove unnecessary IDE configuration files

378e115

[pre-commit.ci] auto fixes from pre-commit.com hooks

dd0d66d

For more information, see https://pre-commit.ci

Refactor excel_test.py for better test independence

da43e14

Merge branch '1357-convert-unittest-to-pytest' of https://github.com/…

4b485a1

…ggurjar333/pudl into 1357-convert-unittest-to-pytest

Refactor excel_test.py for better test independence

291d6af

[pre-commit.ci] auto fixes from pre-commit.com hooks

79fb818

For more information, see https://pre-commit.ci

Refactor excel_test.py for better test independence

35c472b

Refactor excel_test.py for better test perfomance

ce806ca

Refactor excel_test.py for better test independance

b1fc12e

[pre-commit.ci] auto fixes from pre-commit.com hooks

cb4014c

For more information, see https://pre-commit.ci

Refactor classes_test.py for improved test organization

e124ee9

Merge branch '1357-convert-unittest-to-pytest' of https://github.com/…

6aba44d

…ggurjar333/pudl into 1357-convert-unittest-to-pytest

Merge branch 'main' into 1357-convert-unittest-to-pytest

202c3ff

Refactor mocks in classes_test.py for improved readability

e98a8ce

zaneselvans marked this pull request as draft November 1, 2024 15:59

ggurjar333 added 4 commits November 2, 2024 08:08

Convert unittest assertions to pytest style in resource_cache_test.py

b512458

Convert unittest assertions to pytest style in resource_cache_test.py

dd41e1f

Convert unittest assertions to pytest style in resource_cache_test.py

fbd2950

Merge branch 'main' into 1357-convert-unittest-to-pytest

4c0b5c9

ggurjar333 marked this pull request as ready for review November 2, 2024 06:21

Merge branch 'main' into 1357-convert-unittest-to-pytest

946f2d0

zaneselvans requested a review from jdangerx November 8, 2024 18:45

zaneselvans added the testing Writing tests, creating test data, automating testing, etc. label Nov 8, 2024

zaneselvans assigned ggurjar333 Nov 8, 2024

Merge branch 'main' into 1357-convert-unittest-to-pytest

601c18e

jdangerx requested changes Nov 8, 2024

View reviewed changes

ggurjar333 and others added 2 commits November 9, 2024 16:09

Merge branch 'main' into 1357-convert-unittest-to-pytest

eac3ac9

Merge branch 'main' into 1357-convert-unittest-to-pytest

bc9464c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1357 convert unittest to pytest #3931

1357 convert unittest to pytest #3931

ggurjar333 commented Oct 26, 2024 •

edited

Loading

Tasks

To-do list

zaneselvans commented Oct 31, 2024

ggurjar333 commented Nov 1, 2024 •

edited

Loading

zaneselvans commented Nov 1, 2024

ggurjar333 commented Nov 2, 2024

jdangerx left a comment

jdangerx Nov 8, 2024

jdangerx Nov 8, 2024

jdangerx Nov 8, 2024

jdangerx Nov 8, 2024

jdangerx Nov 8, 2024

jdangerx Nov 8, 2024

jdangerx Nov 8, 2024

jdangerx Nov 8, 2024

jdangerx Nov 8, 2024

jdangerx Nov 8, 2024

		"""Tests basic operation of the excel.Metadata object."""

		@pytest.fixture(autouse=True)

	"""Test leadward propagation with undecided children."""
	"""Test leafward propagation with undecided children."""

1357 convert unittest to pytest #3931

Are you sure you want to change the base?

1357 convert unittest to pytest #3931

Conversation

ggurjar333 commented Oct 26, 2024 • edited Loading

Overview

What problem does this address?

What did you change?

Documentation

Tasks

Testing

How did you make sure this worked? How can a reviewer verify this?

To-do list

zaneselvans commented Oct 31, 2024

ggurjar333 commented Nov 1, 2024 • edited Loading

zaneselvans commented Nov 1, 2024

ggurjar333 commented Nov 2, 2024

jdangerx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggurjar333 commented Oct 26, 2024 •

edited

Loading

ggurjar333 commented Nov 1, 2024 •

edited

Loading