Skip to content

Fixed Bug Regarding Attribute Error in pytest.approx For Types Implicitly Convertible to Numpy Arrays#12232

Merged
RonnyPfannschmidt merged 13 commits intopytest-dev:mainfrom
poulami-sau:implicit_array_conversion_fix
Apr 23, 2024
Merged

Fixed Bug Regarding Attribute Error in pytest.approx For Types Implicitly Convertible to Numpy Arrays#12232
RonnyPfannschmidt merged 13 commits intopytest-dev:mainfrom
poulami-sau:implicit_array_conversion_fix

Conversation

@poulami-sau
Copy link
Copy Markdown
Contributor

Fixed the bug for _repr_compare in the ApproxNumpy class located in pytest.approx to ensure that other_side is a numpy array before comparing np_array_shape != other_side.shape. I have verified that the provided test cases, along with the test case provided in #12114, have all passed using pytest testing/python/approx.py.

Copy link
Copy Markdown
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

thanks for getting this started

i suspect we need to setup a test env which includes numpy on github as the lack of coverage comes unexpected to me, i'll try to resolve that today

Comment thread src/_pytest/python_api.py Outdated
)

# convert other_side to numpy array to ensure shape attribute is available
other_side = _as_numpy_array(other_side)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i believe we need to expand the type annotation to include objects which may cast to a array + use a new variable name like other_side_as_array + assert its not none

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alright, I added those changes in a push. Let me know if the typing for other_side looks alright, currently it is set to other_side: Union["ndarray", List[Any]].

Comment thread src/_pytest/python_api.py
@RonnyPfannschmidt
Copy link
Copy Markdown
Member

I presume you would like a squash commit instead of a merge

@poulami-sau
Copy link
Copy Markdown
Contributor Author

Yeah that sounds good to me!

@poulami-sau
Copy link
Copy Markdown
Contributor Author

Hi Ronny, I just wanted to clarify if I have to do anything else for this pull request?

@RonnyPfannschmidt
Copy link
Copy Markdown
Member

No, next step is merge,I'm just preoccupied

@RonnyPfannschmidt RonnyPfannschmidt merged commit 5cffef7 into pytest-dev:main Apr 23, 2024
@RonnyPfannschmidt
Copy link
Copy Markdown
Member

thanks again !

@poulami-sau
Copy link
Copy Markdown
Contributor Author

Of course! Thank you for your help and guidance :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants