Add method to pip test result #7869

ssurbhi560 · 2020-03-17T19:43:59Z

Towards #6050

ssurbhi560 · 2020-03-17T19:45:19Z

Hey @pradyunsg ! Can you please have a look at this. :)

pradyunsg

2 broad comments, one for clarity, one for correctness.

pradyunsg · 2020-03-17T20:47:32Z

tests/functional/test_download.py

-    )
-    assert script.site_packages / 'piptestpackage' not in result.files_created
+    result.did_create(
+        Path('scratch') / 'meta-1.0-py2.py3-none-any.whl')


nit: for clarity

Suggested change

Path('scratch') / 'meta-1.0-py2.py3-none-any.whl')

Path('scratch') / 'meta-1.0-py2.py3-none-any.whl'

)

pradyunsg · 2020-03-17T20:48:47Z

tests/functional/test_install_compat.py

+    result.did_create(egg_info), \
        "Couldn't find {egg_info}".format(**locals())


Hmm, looks like here (and a lot more places) the assertion didn't get translated well.

assert x in Y, message

Got changed to:

result.did_create(x), message

Which isn't right. For now, I suggest undoing the changes in those locations. We'll come back to them in a follow up PR. :)

result.did_create(x), message wouldn't work, right -- Either message needs to be passed as a parameter, or a better option IMHO would be to let result.did_create return a boolean, and assert the correctness in the test function itself. This would also improve it's readability:

assert result.did_create(x), message

@pradyunsg would love to hear what you think on this.

I also support the way @RJ722 suggested. That does seem straightforward. :)

The thing is, we'd want to always have the message include str(result), since it contains useful debugging information. Part of the point of making this change, is to get rid of needing to specify the message every time.

I think we'd likely want to add a message keyword-only argument to result, so that we can add to the default error message:

result.did_create(X, message=message)

That should do:

assert X in self.created, "\n".join(message, str(self))

pradyunsg

There's a whole bunch more left (I didn't actually scroll all the way to the end of this PR).

tests/lib/__init__.py

tests/functional/test_install.py

tests/functional/test_wheel.py

ssurbhi560 · 2020-04-02T17:36:02Z

Thanks for the quick review @pradyunsg ! I got a bit confused with this change, but seems now I understood. I will update the PR. :)

BrownTruck · 2020-04-14T02:00:04Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

deveshks · 2020-05-05T19:14:27Z

Hi @ssurbhi560 ,

Looks like the changes made in this PR are passing CI checks. Once you resolve the merge conflict, I believe this PR will be in a reviewable state again.

deveshks · 2020-05-06T13:25:05Z

tests/lib/__init__.py

+        if message is None:
+            message = str(self)
+        else:
+            message = "\n".join((message, str(self)))


I see that this if-else is being repeated at 4 places. Maybe it's a good idea to move this in it's own function, and use a single line if-else. (Or you can just move it in your own function, and not use the single line if-else for readability)

def create_msg(message=None): return "\n".join((message, str(self))) if message else str(self)

Sorry for bikeshedding, but I think it's more readable to have the messaged string as {}\n{},format(message, self). A bit more importantly, the repetition signals that the assertions are similar enough that parameterizing them might be a good idea.

Sorry for bikeshedding, but I think it's more readable to have the messaged string as {}\n{},format(message, self)

Agreed that return '{}\n{}'.format(message, self) if message else self is a better idea (We don't need to explicitly cast self to str via str(self)

A bit more importantly, the repetition signals that the assertions are similar enough that parameterizing them might be a good idea.

I think it was suggested at #6050 (comment) to have different functions for asserting in and not in of self.files_created and self.files_updated

…not created

…ated

gutsytechster · 2020-05-20T13:06:17Z

@ssurbhi560, I am glad you're back. I was actually working over this PR to resolve the conflict(glad that you've already resolved it) and break it into smaller PRs.
PR can be broke down in 2-3 PRs with

One of them could only add the test methods like assert_did_create etc.
Other PRs may update the test files in batches(keeping 2-3 file per PR) so that single PR doesn't become overwhelming with changes.

Let me know if you are going to keep working over it else I would take over the work. :)

ssurbhi560 · 2020-05-20T15:09:01Z

@gutsytechster, I'd be glad to continue working on this, sorry for the delay that happened this time.
About having a lot of changes in one PR is difficult to review, I agree with you on that. Let's just ask what @pradyunsg has to say over this! :-)

pradyunsg · 2020-05-20T16:05:41Z

What @gutsytechster says makes sense to me! :)

In terms of breaking this down, I think the first PR (adding the methods) is likely the only bit that has to be only-one-person-does-it. The follow up PRs can be independent, and done by different folks, as long as there's some sort of conflict avoidance (eg: one person does all "install" test files, another doing everything else).

BrownTruck · 2020-05-22T11:00:04Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

pradyunsg · 2020-05-22T12:15:44Z

In terms of breaking this down, I think the first PR (adding the methods) is likely the only bit that has to be only-one-person-does-it.

To that end, I've gone ahead and filed #8303 for this. That should unblock us and allow us to incrementally move over all of pip's tests to using the helpers introduced in that PR.

The follow up PRs can be independent, and done by different folks, as long as there's some sort of conflict avoidance (eg: one person does all "install" test files, another doing everything else).

If @ssurbhi560 and @gutsytechster are tackling these, I suggest using the split suggested above. Keep the PRs small, at most two files per PR, and tackle away at this! :)

deveshks · 2020-05-22T14:24:04Z

Now that we have a plan in place on how to break this PR into smaller PRs, I think we can close this PR.

pradyunsg requested changes Mar 21, 2020

View reviewed changes

pradyunsg requested changes Apr 2, 2020

View reviewed changes

BrownTruck added the needs rebase or merge PR has conflicts with current master label Apr 14, 2020

ssurbhi560 force-pushed the Add_method_to_PipTestResult branch from 972438d to 962f46f Compare May 6, 2020 11:39

pypa-bot removed the needs rebase or merge PR has conflicts with current master label May 6, 2020

deveshks reviewed May 6, 2020

View reviewed changes

pradyunsg added the needs rebase or merge PR has conflicts with current master label May 19, 2020

ssurbhi560 added 14 commits May 20, 2020 17:35

Use TestPipResult.did_create for path lookups

291b282

Use PipTestResult.did_not_create method to do path lookups for files …

37f813e

…not created

Use TestPipResult.did_update to do path lookups for updated files

c61fe51

Use PipTestResult.did_not_update to do path lookups for files not upd…

d7f2b3a

…ated

Add News entry file

243f0a3

Update PipTestResult.did_create method and invocations

500dc77

Update PipTestResult.did_not_create method and invocations

8eef066

Update the PipTestResult.did_update method

ff6de3b

Update the PipTestResult.did_not_update method

e187aeb

Update PipTestResult.did_create method and invocations

f5b1f57

Update TestPipResult.did_not_create method and invocations

92aaee8

Update TestPipResult.did_update method and invocations

e579c54

Update TestPipResult.did_not_update method

3ed055e

updating methods

7a2c2d6

ssurbhi560 force-pushed the Add_method_to_PipTestResult branch from 962f46f to 7a2c2d6 Compare May 20, 2020 12:21

pypa-bot removed the needs rebase or merge PR has conflicts with current master label May 20, 2020

deveshks mentioned this pull request May 20, 2020

Remove tests.lib.path #6050

Closed

BrownTruck added the needs rebase or merge PR has conflicts with current master label May 22, 2020

pradyunsg mentioned this pull request May 22, 2020

Add helpers to TestPipResult #8303

Merged

ssurbhi560 closed this May 22, 2020

github-actions bot locked as resolved and limited conversation to collaborators Oct 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method to pip test result #7869

Add method to pip test result #7869

ssurbhi560 commented Mar 17, 2020

ssurbhi560 commented Mar 17, 2020

pradyunsg left a comment

pradyunsg Mar 17, 2020

pradyunsg Mar 17, 2020

RJ722 Mar 21, 2020

gutsytechster Mar 22, 2020

pradyunsg Mar 26, 2020

pradyunsg left a comment

ssurbhi560 commented Apr 2, 2020

BrownTruck commented Apr 14, 2020

deveshks commented May 5, 2020

deveshks May 6, 2020

McSinyx May 6, 2020

deveshks May 6, 2020 •

edited

Loading

gutsytechster commented May 20, 2020 •

edited

Loading

ssurbhi560 commented May 20, 2020

pradyunsg commented May 20, 2020

BrownTruck commented May 22, 2020

pradyunsg commented May 22, 2020

deveshks commented May 22, 2020 •

edited

Loading

	Path('scratch') / 'meta-1.0-py2.py3-none-any.whl')
	Path('scratch') / 'meta-1.0-py2.py3-none-any.whl'
	)

		result.did_create(egg_info), \
		"Couldn't find {egg_info}".format(**locals())

Add method to pip test result #7869

Add method to pip test result #7869

Conversation

ssurbhi560 commented Mar 17, 2020

ssurbhi560 commented Mar 17, 2020

pradyunsg left a comment

Choose a reason for hiding this comment

pradyunsg Mar 17, 2020

Choose a reason for hiding this comment

pradyunsg Mar 17, 2020

Choose a reason for hiding this comment

RJ722 Mar 21, 2020

Choose a reason for hiding this comment

gutsytechster Mar 22, 2020

Choose a reason for hiding this comment

pradyunsg Mar 26, 2020

Choose a reason for hiding this comment

pradyunsg left a comment

Choose a reason for hiding this comment

ssurbhi560 commented Apr 2, 2020

BrownTruck commented Apr 14, 2020

deveshks commented May 5, 2020

deveshks May 6, 2020

Choose a reason for hiding this comment

McSinyx May 6, 2020

Choose a reason for hiding this comment

deveshks May 6, 2020 • edited Loading

Choose a reason for hiding this comment

gutsytechster commented May 20, 2020 • edited Loading

ssurbhi560 commented May 20, 2020

pradyunsg commented May 20, 2020

BrownTruck commented May 22, 2020

pradyunsg commented May 22, 2020

deveshks commented May 22, 2020 • edited Loading

deveshks May 6, 2020 •

edited

Loading

gutsytechster commented May 20, 2020 •

edited

Loading

deveshks commented May 22, 2020 •

edited

Loading