search

pytest-dev / nose2pytest

Scripts to convert Python Nose tests to PyTest
Other
38 stars 12 forks source link

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

Closed massich closed 1 year ago

massich commented 7 years ago

fixes #5

schollii commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Hi Joan, The path is a sequence of indices through the node tree. To figure out the path, You basically set a breakpoint in the transform method, and you use the debugger to figure out the nodes you want in the destination expression, here your destination is "a == approx(b, abs=delta)" but the indices must target a, b and delta so these can be copied from the original expression into the "slots" for the new expression. I use PyCharm -- at the breakpoint, you can expand the node of the target expression to see its children, pick the child of interest which is itself a node, and expand it etc until you get to the nodes of interest). See if that helps. If not, I'll take a closer look at the old code and can provide more precise instructions. Thanks, Oliver

On Mon, 4 Sep 2017 at 04:45 Joan Massich notifications@github.com wrote:

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

massich commented 7 years ago

That's what I did. And delta is at (0,1,1,4) instead of 2 but changing the path to that still makes out of bounds. Which I don't get.

On Mon, Sep 4, 2017, 22:03 schollii notifications@github.com wrote:

Hi Joan, The path is a sequence of indices through the node tree. To figure out the path, You basically set a breakpoint in the transform method, and you use the debugger to figure out the nodes you want in the destination expression, here your destination is "a == approx(b, abs=delta)" but the indices must target a, b and delta so these can be copied from the original expression into the "slots" for the new expression. I use PyCharm -- at the breakpoint, you can expand the node of the target expression to see its children, pick the child of interest which is itself a node, and expand it etc until you get to the nodes of interest). See if that helps. If not, I'll take a closer look at the old code and can provide more precise instructions. Thanks, Oliver

On Mon, 4 Sep 2017 at 04:45 Joan Massich notifications@github.com wrote:

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

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub <https://github.com/pytest-dev/nose2pytest/pull/6#issuecomment-326903522 , or mute the thread < https://github.com/notifications/unsubscribe-auth/ACenmH8SdWMCrTBqYekBvVfDRtUYhkxlks5se7jBgaJpZM4PBotg

.

-- Oliver My StackOverflow contributions My CodeProject articles My Github projects My SourceForget.net projects

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pytest-dev/nose2pytest/pull/6#issuecomment-327021951, or mute the thread https://github.com/notifications/unsubscribe-auth/AGt-48TIM_PlYOZfXsZLTjVOCqbsMNrGks5sfFd3gaJpZM4PBotg .

schollii commented 7 years ago

If so then I can have a look at it.

On Mon, 4 Sep 2017 at 21:54 oliver oliver.schoenborn@gmail.com wrote:

Did you commit your latest code on your branch?

On Mon, 4 Sep 2017 at 16:55 Joan Massich notifications@github.com wrote:

That's what I did. And delta is at (0,1,1,4) instead of 2 but changing the path to that still makes out of bounds. Which I don't get.

On Mon, Sep 4, 2017, 22:03 schollii notifications@github.com wrote:

Hi Joan, The path is a sequence of indices through the node tree. To figure out the path, You basically set a breakpoint in the transform method, and you use the debugger to figure out the nodes you want in the destination expression, here your destination is "a == approx(b, abs=delta)" but the indices must target a, b and delta so these can be copied from the original expression into the "slots" for the new expression. I use PyCharm -- at the breakpoint, you can expand the node of the target expression to see its children, pick the child of interest which is itself a node, and expand it etc until you get to the nodes of interest). See if that helps. If not, I'll take a closer look at the old code and can provide more precise instructions. Thanks, Oliver

On Mon, 4 Sep 2017 at 04:45 Joan Massich notifications@github.com wrote:

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 commented 7 years ago

Did you commit your latest code on your branch?

On Mon, 4 Sep 2017 at 16:55 Joan Massich notifications@github.com wrote:

That's what I did. And delta is at (0,1,1,4) instead of 2 but changing the path to that still makes out of bounds. Which I don't get.

On Mon, Sep 4, 2017, 22:03 schollii notifications@github.com wrote:

Hi Joan, The path is a sequence of indices through the node tree. To figure out the path, You basically set a breakpoint in the transform method, and you use the debugger to figure out the nodes you want in the destination expression, here your destination is "a == approx(b, abs=delta)" but the indices must target a, b and delta so these can be copied from the original expression into the "slots" for the new expression. I use PyCharm -- at the breakpoint, you can expand the node of the target expression to see its children, pick the child of interest which is itself a node, and expand it etc until you get to the nodes of interest). See if that helps. If not, I'll take a closer look at the old code and can provide more precise instructions. Thanks, Oliver

On Mon, 4 Sep 2017 at 04:45 Joan Massich notifications@github.com wrote:

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

massich commented 7 years ago

The branch is clean you can give it a look

On Tue, Sep 5, 2017, 03:55 schollii notifications@github.com wrote:

Did you commit your latest code on your branch?

On Mon, 4 Sep 2017 at 16:55 Joan Massich notifications@github.com wrote:

That's what I did. And delta is at (0,1,1,4) instead of 2 but changing the path to that still makes out of bounds. Which I don't get.

On Mon, Sep 4, 2017, 22:03 schollii notifications@github.com wrote:

Hi Joan, The path is a sequence of indices through the node tree. To figure out the path, You basically set a breakpoint in the transform method, and you use the debugger to figure out the nodes you want in the destination expression, here your destination is "a == approx(b, abs=delta)" but the indices must target a, b and delta so these can be copied from the original expression into the "slots" for the new expression. I use PyCharm -- at the breakpoint, you can expand the node of the target expression to see its children, pick the child of interest which is itself a node, and expand it etc until you get to the nodes of interest). See if that helps. If not, I'll take a closer look at the old code and can provide more precise instructions. Thanks, Oliver

On Mon, 4 Sep 2017 at 04:45 Joan Massich notifications@github.com wrote:

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

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub < https://github.com/pytest-dev/nose2pytest/pull/6#issuecomment-326903522 , or mute the thread <

https://github.com/notifications/unsubscribe-auth/ACenmH8SdWMCrTBqYekBvVfDRtUYhkxlks5se7jBgaJpZM4PBotg

.

-- Oliver My StackOverflow contributions My CodeProject articles My Github projects My SourceForget.net projects

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/pytest-dev/nose2pytest/pull/6#issuecomment-327021951 , or mute the thread <

https://github.com/notifications/unsubscribe-auth/AGt-48TIM_PlYOZfXsZLTjVOCqbsMNrGks5sfFd3gaJpZM4PBotg

.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub <https://github.com/pytest-dev/nose2pytest/pull/6#issuecomment-327027110 , or mute the thread < https://github.com/notifications/unsubscribe-auth/ACenmEQ1wGcyI11IshxAGRkku2h3KzTEks5sfGPMgaJpZM4PBotg

.

-- Oliver My StackOverflow contributions My CodeProject articles My Github projects My SourceForget.net projects

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pytest-dev/nose2pytest/pull/6#issuecomment-327051493, or mute the thread https://github.com/notifications/unsubscribe-auth/AGt-4z43ZPNOKlG7YcqlsvKfRRVe7NGWks5sfKoMgaJpZM4PBotg .

massich commented 6 years ago

@schollii any hints?

schollii commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

\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.\

massich commented 6 years ago

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

schollii commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

massich commented 6 years ago

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 commented 6 years ago

Have you tried running it on your code base to see that it converts all the almostequals properly? I've uncovered bugs in the past when I did that, there's nothing like a real code base!

On Mon, Nov 27, 2017, 09:31 Joan Massich, notifications@github.com wrote:

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.

massich commented 6 years ago

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 commented 6 years ago

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

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

massich commented 6 years ago

I think we can integrate this one.

schollii commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

nicoddemus commented 2 years ago

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

schollii commented 2 years ago

Sure I'll have a look at merging this

schollii commented 1 year ago

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 commented 1 year ago

@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).