TST: Get tests to run and fix them to pass #50636

phershbe · 2023-01-09T05:20:32Z

NOTE: test_metadata_propagation is still not fixed yet in this draft pull request

Changed the class name from Generic to TestGeneric in order to get the test to run and then fixed five groups of tests (test_rename, test_get_numeric_data, test_frame_or_series_compound_dtypes, test_metadata_propagation, test_api_compat) in order to make sure that all of the tests pass.

phershbe · 2023-01-09T05:51:54Z

@MarcoGorelli Thank you for your patience, I'm learning a lot. The work is explained in the description above. There are five groups of tests that weren't passing and four of them are fixed.

test_metadata_propagation on Line 174 still isn't fixed. I understand the basic idea, that we are assigning name variables to sets of data and then passing those sets of data through operations and making sure that the metadata is still the same. I know that assert_metadata_equivalent always needs to compare two sets of data, and sometimes here the second set is not added yet, but I can't figure out yet what to add. I'm sure that I can get it with some more time, but I've taken a long time already, so feel free to do it if you want.

Also, the reset_index(drop=True) on Line 96 works to get the test using a Series in test_get_numeric_data to pass, but it seems like there might be some cleaner way.

MarcoGorelli

Nice work @phershbe !

For the metadata test, here's how I'd approach it:

tm.assert_metadata_equivalent(result) doesn't run, so let's check which PR last touched it:

$ git log -G 'tm.assert_metadata_equivalent\(result\)'
commit 3570efd439ae49a7800f63a6247f089fa462405a
Author: Matthew Roeschke <>
Date:   Tue Dec 28 12:32:31 2021 -0800

    TST: More pytest idioms in tests/generic (#45086)

let's look at TST: More pytest idioms in tests/generic #45086 - we see that

    def check_metadata(self, x, y=None):
        for m in x._metadata:
            v = getattr(x, m, None)
            if y is None:
                assert v is None
            else:
                assert v == getattr(y, m, None)

became

def assert_metadata_equivalent(left, right):
    """
    Check that ._metadata attributes are equivalent.
    """
    for attr in left._metadata:
        val = getattr(left, attr, None)
        if right is None:
            assert val is None
        else:
            assert val == getattr(right, attr, None)

Looks like there was a little difference between the functions, but it wasn't picked up because the tests weren't running - can you spot it?
finally, it would be good to add type annotations to this function, whilst we're changing it. I think just DataFrame | Series should work

phershbe · 2023-01-10T05:01:31Z

@MarcoGorelli Whoa, you're the best, thank you for the guidance. The advice to find which PR last touched the code and then investigate it should be very helpful for me in the future, thank you. I updated the code and the tests all pass now!

I didn't add anything to the doc/source/whatsnew/vX.X.X.rst because I thought that maybe this is too small of an issue, but I can if you think it's a good idea. Let me know.

Also, after my initial commit, I got some e-mails saying that various checks in CI were failing, is that normal?

Of course, feel free to let me know if there is anything else that needs to be changed.

MarcoGorelli

nice!

yeah you can see the failing check here: https://github.com/pandas-dev/pandas/actions/runs/3880306936/jobs/6618255248

can you also remove the exclude from

pandas/.pre-commit-config.yaml

Lines 446 to 448 in 0fa40d8

    
                   exclude: | 
        
                       (?x) 
        
                       ^pandas/tests/generic/test_generic.py  # GH50380

please?

pandas/_testing/asserters.py

…test-naming

phershbe · 2023-01-10T19:21:49Z

@MarcoGorelli Great, thank you for the reminder about reading about the failed check. On a similar note, there is already a failed check here, but it's something about Ubuntu that is failing on a lot of other pull requests from other people I noticed.

I'm trying to read about the meaning of the | and (?x) in exclude in YAML in order to better understand what I'm doing, but I went ahead and deleted all three lines because it seems like that's what you were asking for.

MarcoGorelli

thanks @phershbe - can you mark as ready for review please? should be good to go

for exclude, check the pre-commit docs

MarcoGorelli

actually, blocking as there's something I hadn't picked up on, which we should check

MarcoGorelli · 2023-01-10T21:28:36Z

pandas/tests/generic/test_generic.py

        result = o._get_bool_data()
-        expected = construct(n, value="empty", **kwargs)
+        expected = construct(frame_or_series, n, value="empty", **kwargs)
        if isinstance(o, DataFrame):
            # preserve columns dtype
            expected.columns = o.columns[:0]
-        tm.assert_equal(result, expected)
+        tm.assert_equal(result.reset_index(drop=True), expected)


@topper-123 is this right?

In [1]: ser = Series([1,2,3]) In [2]: ser.index Out[2]: RangeIndex(start=0, stop=3, step=1) In [3]: ser._get_bool_data() Out[3]: Series([], dtype: int64) In [4]: ser._get_bool_data().index Out[4]: Index([], dtype='object')

@phershbe OK for now could you please just add a comment with

# https://github.com/pandas-dev/pandas/issues/50862

above this line? then we can get this in and address that separately if needed

phershbe · 2023-01-11T01:41:43Z

thanks @phershbe - can you mark as ready for review please? should be good to go

for exclude, check the pre-commit docs

@MarcoGorelli I'll wait for the response on the part that's blocking in the comment above before marking as ready because maybe that will inform another reviewer that it's ready I guess, but I'll check back in a few hours at the end of the night and again in the morning and mark it ready when you let me know so that it can get finished since it has taken me so long.

MarcoGorelli

Just left a comment

If you fetch and merge upstream/main and mark as ready for review then we can get this in

MarcoGorelli · 2023-01-19T11:54:27Z

pandas/tests/generic/test_generic.py

        result = o._get_bool_data()
-        expected = construct(n, value="empty", **kwargs)
+        expected = construct(frame_or_series, n, value="empty", **kwargs)
        if isinstance(o, DataFrame):
            # preserve columns dtype
            expected.columns = o.columns[:0]
-        tm.assert_equal(result, expected)
+        tm.assert_equal(result.reset_index(drop=True), expected)


@phershbe OK for now could you please just add a comment with

# https://github.com/pandas-dev/pandas/issues/50862

above this line? then we can get this in and address that separately if needed

phershbe · 2023-01-19T16:02:05Z

@MarcoGorelli Thank you, I'm on it, doing it now

phershbe · 2023-01-19T17:59:57Z

@MarcoGorelli Okay, I think everything should be ready now. I was a little bit confused because your message here in the conversation was a comment about a comment in the files changed tab about adding a comment to the code ... it's clear enough though, it's my inexperience. I added the comment above Line 91, I didn't know if it would be more appropriate there or above Line 97, so feel free to move it if necessary.

pandas/tests/generic/test_generic.py

MarcoGorelli

Thanks @phershbe !

* updated tests * updated assert_metadata_equivalent * updated assert_metadata_equivalent and deleted exclude in YAML check-test-naming * added comment * move comment Co-authored-by: Marco Edward Gorelli <33491632+MarcoGorelli@users.noreply.github.com>

updated tests

c9c557f

MarcoGorelli requested changes Jan 9, 2023

View reviewed changes

updated assert_metadata_equivalent

7b67f8f

MarcoGorelli requested changes Jan 10, 2023

View reviewed changes

pandas/_testing/asserters.py Outdated Show resolved Hide resolved

updated assert_metadata_equivalent and deleted exclude in YAML check-…

85d90ab

…test-naming

MarcoGorelli approved these changes Jan 10, 2023

View reviewed changes

MarcoGorelli added this to the 2.0 milestone Jan 10, 2023

MarcoGorelli added the Testing pandas testing functions or related to the test suite label Jan 10, 2023

MarcoGorelli requested changes Jan 10, 2023

View reviewed changes

MarcoGorelli mentioned this pull request Jan 19, 2023

API should empty ._get_bool_data have RangeIndex? #50862

Open

MarcoGorelli requested changes Jan 19, 2023

View reviewed changes

added comment

6bb8a20

phershbe marked this pull request as ready for review January 19, 2023 17:50

MarcoGorelli reviewed Jan 19, 2023

View reviewed changes

pandas/tests/generic/test_generic.py Outdated Show resolved Hide resolved

move comment

f9969a7

MarcoGorelli approved these changes Jan 21, 2023

View reviewed changes

MarcoGorelli merged commit 56c1b20 into pandas-dev:main Jan 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TST: Get tests to run and fix them to pass #50636

TST: Get tests to run and fix them to pass #50636

phershbe commented Jan 9, 2023 •

edited

phershbe commented Jan 9, 2023

MarcoGorelli left a comment •

edited

phershbe commented Jan 10, 2023

MarcoGorelli left a comment

phershbe commented Jan 10, 2023

MarcoGorelli left a comment •

edited

MarcoGorelli left a comment

MarcoGorelli Jan 10, 2023

MarcoGorelli Jan 19, 2023

phershbe commented Jan 11, 2023 •

edited

MarcoGorelli left a comment

MarcoGorelli Jan 19, 2023

phershbe commented Jan 19, 2023

phershbe commented Jan 19, 2023

MarcoGorelli left a comment

	exclude: \|
	(?x)
	^pandas/tests/generic/test_generic.py # GH50380

TST: Get tests to run and fix them to pass #50636

TST: Get tests to run and fix them to pass #50636

Conversation

phershbe commented Jan 9, 2023 • edited

phershbe commented Jan 9, 2023

MarcoGorelli left a comment • edited

Choose a reason for hiding this comment

phershbe commented Jan 10, 2023

MarcoGorelli left a comment

Choose a reason for hiding this comment

phershbe commented Jan 10, 2023

MarcoGorelli left a comment • edited

Choose a reason for hiding this comment

MarcoGorelli left a comment

Choose a reason for hiding this comment

MarcoGorelli Jan 10, 2023

Choose a reason for hiding this comment

MarcoGorelli Jan 19, 2023

Choose a reason for hiding this comment

phershbe commented Jan 11, 2023 • edited

MarcoGorelli left a comment

Choose a reason for hiding this comment

MarcoGorelli Jan 19, 2023

Choose a reason for hiding this comment

phershbe commented Jan 19, 2023

phershbe commented Jan 19, 2023

MarcoGorelli left a comment

Choose a reason for hiding this comment

phershbe commented Jan 9, 2023 •

edited

MarcoGorelli left a comment •

edited

MarcoGorelli left a comment •

edited

phershbe commented Jan 11, 2023 •

edited