Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

poulami-sau
Copy link
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
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

@@ -163,6 +163,9 @@ def get_value_from_nested_list(
self._approx_scalar, self.expected.tolist()
)

# convert other_side to numpy array to ensure shape attribute is available
other_side = _as_numpy_array(other_side)
Copy link
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
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]].

@RonnyPfannschmidt
Copy link
Member

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

@poulami-sau
Copy link
Contributor Author

Yeah that sounds good to me!

@poulami-sau
Copy link
Contributor Author

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

@RonnyPfannschmidt
Copy link
Member

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

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

thanks again !

@poulami-sau
Copy link
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