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.

Sign up for GitHub

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

Jump to bottom

transform assert_almost_equal (a,b) into assert a == approx(b) #6

Merged
merged 12 commits into from
Nov 17, 2022
Merged

transform assert_almost_equal (a,b) into assert a == approx(b) #6

merged 12 commits into from
Nov 17, 2022

Conversation

massich
Copy link
Contributor

@massich massich commented Aug 24, 2017

fixes #5

@schollii
Copy link
Collaborator

schollii commented Sep 3, 2017

Hi @massich thanks for contributing a PR. Github is showing me that you created a new class but this class is not used anywhere in code base, is that what you intended? I expected to see each file listed on https://github.com/pytest-dev/nose2pytest/search?utf8=%E2%9C%93&q=assert_almost_equal&type= to need fixing to support the switch to approx. And since the change is on a branch, there is no risk.

@massich
Copy link
Contributor Author

massich commented Sep 4, 2017

I had create this class you are talking about when we were looking to have both implementations (before and after pytest-3.0) but in the reference issue we decided to not add the option for legacy transformation unless requested.

Once said that, I could make use of some help to make this PR to work. I changed the test for almost equal and changed its definition however when parsing the node I get an error. I've been trying to understand _get_node() in order to provide a correct path for the new translation but I don't really get it. When I inspect the node manually the delta value is at (0,1,1,4) instead of 2 but this does not make sense either.

Any hints are wellcome

@schollii
Copy link
Collaborator

schollii commented Sep 4, 2017 via email •
edited
Loading

@massich
Copy link
Contributor Author

massich commented Sep 4, 2017 via email

@schollii
Copy link
Collaborator

schollii commented Sep 5, 2017 via email •
edited
Loading

@schollii
Copy link
Collaborator

schollii commented Sep 5, 2017 via email •
edited
Loading

@massich
Copy link
Contributor Author

massich commented Sep 5, 2017 via email

@massich
Copy link
Contributor Author

massich commented Nov 3, 2017

@schollii any hints?

@schollii
Copy link
Collaborator

schollii commented Nov 3, 2017

Sorry Massich just been very busy with work. I'll have a look after work today or during the weekend, i'd really like to close this because it'll probably indicate missing documentation or hopefully a simple tweak will be sufficient to cover this case.

@massich
Copy link
Contributor Author

massich commented Nov 3, 2017

I didn't manage to understand properly the structure of lib2to3.pytree and how DEFAULT_ARG_PATHS is related to _get_node (which seems to do what I expect when I call the function directly.

I was planning to use hypothesis package to generate DEFAULT_ARG_PATHS candidates and brake it with brute force, but I didn't succeed either.

@schollii
Copy link
Collaborator

schollii commented Nov 6, 2017

@massich the paths were actually the easy part to fix, I will add a description of steps in the readme doc on how to do that. The tougher part was that once I had fixed the paths, the spacing was wrong: assert_almost_eq(a, b, c) should become a == pytest.approx(b, abs=c) whereas it was actually becoming a == pytest.approx( b, abs= c). I fixed that.

I'm still new to collab on github (I haven't had to deal with PR's until now). If I had not had to make adjustments, I could just merge your PR and close it. I had to make changes, so if I commit those and push them to my master branch, I end up having to reject your PR and close it, which I rather not do because then it will seem like your contribution was rejected. Any ideas?

@massich
Copy link
Contributor Author

massich commented Nov 6, 2017 •
edited
Loading

Actually its really easy, since you have full rights you can push directly to the PR branch. Your commits would go on the top of mine. In this manner if there's something else to be done like write some more tests I can take over.

@schollii
Copy link
Collaborator

schollii commented Nov 6, 2017

No kidding, that's pretty cool. So what I did so far is this:

  1. on my dev machine, created and switched to a branch (git checkout -b massich-almost_equal_to_approx master)
  2. pulled your PR branch (git pull git://github.com/massich/nose2pytest.git almost_equal_to_approx)
  3. edited the code

I did not yet commit my edits yet, so it sounds like I would do the following:

  1. commit my edits to my checked out repo
  2. push committed changes to your fork on your PR branch (instead of master branch on origin)

From there we would both work like this on your PR branch, then eventually I would accept and merge your PR. When I merge it via the github web UI, does git automatically use the HEAD of the PR branch?

BTW do you know of a good explanation of this workflow somewhere, the github book only seems to cover the case of "contributor-creates-PR-then-author-merges-it".

@massich
Copy link
Contributor Author

massich commented Nov 6, 2017

4 and 5 seems like the right thing. If there's any trouble I give you rights on my fork, but you should not need them since you own the main project. (see here) Then when you click the green button everything would be squashed and merged :) and we both would own the changes. And if we ever need to get back to it, github keeps the history of the squash :) so that we can restore the PR even after closed.

@schollii
Copy link
Collaborator

schollii commented Nov 7, 2017

I tried git push -n https://github.com/massich/nose2pytest.git almost_equal_to_approx but that did not work:

error: src refspec almost_equal_to_approx does not match any.
error: failed to push some refs to 'https://github.com/massich/nose2pytest.git'

If I add a colon before the branch name, the output is the following:

To https://github.com/massich/nose2pytest.git
 - [deleted]         almost_equal_to_approx

Why is it saying your branch has been deleted? Note the -n as I just wanted to try the commands.

@massich
Copy link
Contributor Author

massich commented Nov 7, 2017 •
edited
Loading

I did a simple test with a collegue, and I had no problem.
1 - I've created a dummy project here.
2 - got a PR
3 - fetch the PR using hub
hub checkout https://github.com/massich/test_push_in_pr/pull/1
4 - do some changes
5 - push to the PR
git push -v git\@github.com\:jorisvandenbossche/test_push_in_pr.git jorisvandenbossche-patch-1\:refs/heads/patch-1

Either way it is strange that you cannot push on the PR, maybe its because the project belongs to a pytest-dev and you might not have rights or something. We can try to figure it out. Can you create a dummy project on your account, I'll PR and you try to commit to the PR.
Meanwhile I added you as a contributor to my fork, once you accept it I think you would be able to push anywhere on my fork.

edit: Actually I've create a PR to one of your projects here

@schollii
Copy link
Collaborator

schollii commented Nov 9, 2017 •
edited
Loading

Figured it out based on https://stackoverflow.com/questions/8479559/github-pushing-to-pull-requests:

$ git push https://github.com/massich/nose2pytest.git massich-almost_equal_to_approx:almost_equal_to_approx
Counting objects: 19, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (18/18), done.
Writing objects: 100% (19/19), 10.35 KiB | 0 bytes/s, done.
Total 19 (delta 13), reused 0 (delta 0)
remote: Resolving deltas: 100% (13/13), completed with 8 local objects.
To https://github.com/massich/nose2pytest.git
   f9ebe80..e2b7baf  massich-almost_equal_to_approx -> almost_equal_to_approx

Looks like Github is tracking that branch as this PR page now indicates that all checks pass, pretty cool.

The only thing that is not clear is whether this would have worked without you having given me permission. It would be worth trying again: if you remove permission you added, I will push another mod.

<rant>I've been using RCS then CVS then Subversion for years and I must say, there are some very cool things about git, but a lot of things go way overboard in complexity. Simple things should be simple to do, complicated things should be possible. Some simple things are simple: branching, merging. But lots of other simple things are not, like what we were trying to do with this PR. And various related discussions on stackoverflow mention rebasing and forcing and leases, makes my head explode. I'm willing to believe that as I use git more, I'll be fine with those things, but it seems like it could be costing me way less time.</rant>

@massich
Copy link
Contributor Author

massich commented Nov 9, 2017

git is a great piece of software. I've removed you from my fork.

@schollii
Copy link
Collaborator

schollii commented Nov 10, 2017 •
edited
Loading

It worked still, good to know.

How about I extend the docs to discuss node paths, then you have a look (add some review comments -- I'd love to try github's collaborative review feature -- and, if you wish, make whatever fixes), then we should be good to merge and close the PR?

@schollii
Copy link
Collaborator

If you could take a quick look at the updated docs (README.rst file), that would be much appreciated. I'm hoping we can close this PR soon then I can create another release.

@massich
Copy link
Contributor Author

massich commented Nov 13, 2017

good. I'll try to give it a go either today or tomorrow.

@massich
Copy link
Contributor Author

massich commented Nov 27, 2017

Sorry I didn't got to it earlier. I would change the way this is explained. But I'm not sure it is worth the effort. What I mean is that I guess that with what you wrote I would had enough to figure out how to modify what I was needing.

So I would get it in as it is. And then change the documentation if someone issues an issue or asks questions. So far, the only one who had asked something has been me. And I wish I had the little section you wrote.

Thanks a lot.

@schollii
Copy link
Collaborator

schollii commented Nov 27, 2017 via email •
edited
Loading

@massich
Copy link
Contributor Author

massich commented Nov 27, 2017

it is actually not working at all :) it doesn't get triggered. And more over what I was trying to translate was not even nose.tools it was from numpy.

I'm using this random code to try

@schollii
Copy link
Collaborator

I checked the code, but can't run the script on it from here so I'm just guessing:

  • The assert_almost_equal calls with 2 args don't match because there is no support for 2-arg assert_almost_equal. It may be easy, do you want to give it a shot.
  • The calls with 3 args don't get converted because the arg name is "decimals" rather than "delta". This either causes

It may be worth merging this pull request and then creating a separate one for the above if you dig into it.

massich
massich commented Nov 28, 2017
View reviewed changes
nose2pytest/script.py Outdated
wrapped_delta_val = wrap_parens_for_comparison(delta_val)
dest3.replace(wrapped_delta_val)

elif delta.children[0] == PyLeaf(token.NAME, 'places'):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've never modified the PATTERN_ALMOST_ARG.

nose2pytest/nose2pytest/script.py

Lines 74 to 78 in 304d875

PATTERN_ALMOST_ARGS = """
power< '{}' trailer< '('
( arglist< aaa=any ',' bbb=any ',' delta=any [','] >
| arglist< aaa=any ',' bbb=any ',' delta=any ',' msg=any > )
')' > >

massich
massich commented Nov 28, 2017
View reviewed changes
tests/test_script.py Outdated
check_transformation('assert_almost_equal(123.456, 124, decimal=7)',
'assert 123.456 == pytest.approx(124, abs=1e-7)')
# check_passes(refac,
# 'assert_almost_equal(123.456, 123.450, decimal=1)',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To use this one, we need to add numpy's assert_almost_equal. I need to check how to do that. Meanwhile the transformation is already ok.

What I'm not sure is the same as in the case of places, which if we compare 123.456 to 123.400 up to the first decimial it fails. Not sure why, maybe I'm not understanding the behavior of decimal or place properly

Copy link
Collaborator

Choose a reason for hiding this comment

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

One issue that I had not thought of is that the script is meant to convert nose assertions to pytest assertions. Including assertions from other tools (like numpy) opens a can of worms, and we have to think this through. What is the relationship there, does e.g. numpy explicitely support nose assertions? (in this case the policy could be that any lib that supports nose assertions can be supported).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the signature of almost equal is almost the same. see here. It only changes places parameter by decimal, and adds an extra parameter verbose.

Another thing that I was thinking was that it might be configured which assertions to translate. In case you only want to target some specific transformation. For instance to split the changes across commits or simply because you prefer to keep some asserts that had been rewritten by some other package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we can merge to master removing this part of numpy, publish the release and keep working on it. We would support nosetest and I can add this part on my fork.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But what is the motivation for transforming a numpy assertion to a raw assertion? The point of doing that when using pytest is that pytest traps raw assertions and prints useful info, so after the conversion one can eliminate nose as a dependency.

But numpy assertions are designed to work with numpy data structures (arrays etc) so typically there would not be a good way of expressing them as equivalent raw assertions. For example the numpy.test.assert_almost_equal would almost certainly work with numpy arrays of any dimension, as well as scalars. So it would likely be incorrect to transform all occurrences of numpy's assert_almost_equal.

I think it makes sense to transform the nose.tools.assert_almost_equal to use pytest's approx, but the numpy one, I just don't see how it belongs in nose2pytest. Are you actually able to use your latest version on your code base to your satisfaction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually you are right. Changing numpy's might not bring usefull information. So how do we ensure that it acutally translate only those comming from nosetest?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not possible at the AST stage to know which module the assert_almost_equal belongs to. There fore, there is no way of preventing inadvertent transformations when the same symbol name is defined in more than one module (such as nose.tools and numpy.tests). There is also no reliable way of knowing whether the arg is a scalar or numpy array. Based on https://docs.scipy.org/doc/numpy-1.13.0/reference/generated/numpy.testing.assert_almost_equal.html, it would therefore be imprudent to transform an assert_almost_equal that has a "places" or "decimals" argument.

In fact, this whole discussion makes me realize that the code in script.py should be extended to return the assertion node unchanged if there are arg names that are not recognized as being those of a nose assertion. This would apply to all nose assertions, not just assert_almost_equal. I'm guessing that such check would be done in the transform(), based on an "allowed arg names" field defined for each assertion, by checking the keys in "results" dict, although part of the check implementation may have to be in the derived class.

So unless I'm misunderstanding your argument, the only part of your latest commit that can be kept for this PR is the version command line ARG. If you'd like to try the arg names check, please do it as a separate PR.

@massich
Copy link
Contributor Author

massich commented Dec 1, 2017

I think we can integrate this one.

auvipy
auvipy approved these changes Sep 17, 2018
View reviewed changes
Copy link

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

Looking forward to getting this merged!

@schollii
Copy link
Collaborator

@cclauss hey do I need to merge this or will one of you guys do it when you are ready for next pytest release?

@cclauss
Copy link
Contributor

cclauss commented Jul 14, 2022

I am not a maintainer of this repo so the checkmark on my review is black, not green. This PR needs the approval of a maintainer.

@schollii
Copy link
Collaborator

@cclauss I'm the author of this repo so I might be able to approve it, but I am not familiar with the pytest approval process, so let me know if I can speed this up

@nicoddemus
Copy link
Member

I can chime in: the nose2pytest repository is its own, independent from pytest, so the maintainer(s) can decide how the approval/release process works.

In general we (the pytest maintainers) suggest you have at least 1 approval and passing CI before merging a PR.

@cclauss
Copy link
Contributor

cclauss commented Jul 17, 2022 •
edited
Loading

OK. Recommended steps based on the last two comments on this thread...

@nicoddemus
Copy link
Member

@schollii would you like to do the honors? Otherwise I will go ahead and approve/merge/release in the next few days.

@schollii
Copy link
Collaborator

schollii commented Jul 18, 2022 •
edited
Loading

Sure I'll have a look at merging this

@schollii
Copy link
Collaborator

I finally got around to checking this. It no longer passes because pytest no longer has pytest_namespace, and also the docs discuss the assert_almost_equal/not_equal functions incorrectly for this PR, and finally there are more situations involving these assertions that need coverage: the cases where no arg given (so places defaults to 7).

This was a bit tricky to fix but not too bad, just needed a bit of refamil. I'm about to push a fix onto this PR.

@schollii schollii merged commit f0c3457 into pytest-dev:master Nov 17, 2022
@schollii
Copy link
Collaborator

@massich @nicoddemus @cclauss Please see my previous comment to understand the following :)

Fixes done and all tests now pass. I couldn't resolve the conflicts via the fork so I did it by pulling massich changes into my repo. Good news is that @massich commits show up as his, and this PR shows up as merged.

I'll address any failures with tox, if that is still in use (it's been such a long time, and I saw a commit message about github-actions somewhere).

This was referenced Apr 30, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schollii schollii schollii left review comments

@auvipy auvipy auvipy approved these changes

@cclauss cclauss cclauss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

almost equal
5 participants
@massich @schollii @cclauss @nicoddemus @auvipy