Summary

rbt fails to post a review from Perforce client - regression from 0.7.4

Review Request #7751 — Created Nov. 2, 2015 and discarded Nov. 2, 2015, 11 p.m.

Information

Owner

a.tinsmith

Repository

RBTools

Branch

release-0.7.x

Bugs

4003

Depends On

Reviewers

Groups

rbtools

People

Description

rbt will fail (with Python exception) to post review based on Perforce working directory for revision range specified like this:

rbt post //repo/path/...@123,@456

Note: My familiarity with the code base is poor and most likely this fix is NOT good for production. I mentioned it in the bug description. I post this review so you guys can fix it properly.

The comments section in the get_changenum method says that revisions dict comes from parse_revision_spec. So, it's likely that the real problem is in the parse_revision_spec, or in the way its return value is stored and later reused with get_changenum. I have no idea, really.

The fix I post here (as per Barret's request in the ticket response) prevents RBTools from failing so devs in my team can still use it for posting reviews.

I ran unit tests as recommended here: https://www.reviewboard.org/docs/codebase/dev/getting-started/
I don't know why so many of them are failing. Maybe my environment is missing some components. I also don't have all relevant SCM tools on my machine (we only use Perforce).

So far my team has great difficulties using RB with Perforce. We can't use P4 generated diffs neither in unified diff format, nor in Perforce format, see https://hellosplat.com/s/beanbag/tickets/4002/ .
We're trying to use lastest RBTools, but as mentioend above the tool fails, as well.

Testing Done

Only Perforce related tests are shown.

$ nosetests -v
...
Testing P4Wrapper.counters ... ok
Testing P4Wrapper.info ... ok
Testing PerforceClient.normalize_exclude_patterns ... ok
Testing PerforceClient.diff with a submitted changelist ... ERROR
Testing PerforceClient.diff with moved files and capability off ... ERROR
Testing PerforceClient.diff with moved files and capability on ... ERROR
Testing PerforceClient.diff with a pending changelist ... ERROR
Testing PerforceClient.parse_revision_spec with invalid specifications ... ok
Testing PerforceClient.parse_revision_spec with no specified revisions ... ERROR
Testing PerforceClient.parse_revision_spec with a pending changelist ... ok
Testing PerforceClient.parse_revision_spec with a shelved changelist ... ok
Testing PerforceClient.parse_revision_spec with a submitted changelist ... ok
Testing PerforceClient.parse_revision_spec with two changelists ... ok
Testing PerforceClient.get_repository_info ... ok
Testing PerforceClient.get_repository_info outside client root ... ok
Testing PerforceClient.scan_for_server_counter with reviewboard.url ... ok
Testing PerforceClient.scan_for_server_counter with encoded reviewboard.url.http:|| ... ok
...

Issues

Description	From	Last Updated
This is a syntax error. Can you run through a series of tests with this change, and update the Description, …	chipx86	Nov. 2, 2015, 10:13 p.m.

Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/perforce.py

Branch:: release-0.7.x
Bugs:: 4003

rbtools/clients/perforce.py (Diff revision 1)

The issue has been resolved. Show all issues

This is a syntax error.

Can you run through a series of tests with this change, and update the Description, Testing Done, and Bug number with all the necessary info? Specifically, I want to have info on why this failed, what situations it caused to fail, and why the fix fixes it, along with detailed testing information showing its prior failure and fix.

a.tinsmith Nov. 2, 2015, 10:13 p.m.

Detailed info on why this failed is under the bug ticket. It includes instructions how to reproduce the failure with detailed information about P4 client/server versions and RBTools version.

brennie Nov. 2, 2015, 10:26 p.m.

That may be, but the change summary and description (which becomes the commit) should state, at minimum, the problem and how it was fixed (in a high level way). You may want to have a look at https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/

Your testing done isn't very useful. Usually it suffices to say "Ran unit tests." for running the unit tests, but many many tests are failing with this patch.

chipx86 Nov. 2, 2015, 10:32 p.m.

Alex: It really helps us to have it here for the reasons Barret stated. To elaborate further, we strive to have all relevant details in our commit history, which helps contributors to understand the purpose for a change without having to read other things, helps us when writing release notes, and helps when grepping commits for related keywords. If you look through our commit history, you'll see all commits are detailed and follow the guidelines in the link above.

Thanks!

Change Summary:

Fixed syntax error.

Testing Done:

	+	Only Perforce related tests are shown.
	+
	+
	+
	+	`$ nosetests -v`
	+	...
	+	Testing P4Wrapper.counters ... ok
	+	Testing P4Wrapper.info ... ok
	+	Testing PerforceClient.normalize_exclude_patterns ... ok
	+	Testing PerforceClient.diff with a submitted changelist ... ERROR
	+	Testing PerforceClient.diff with moved files and capability off ... ERROR
	+	Testing PerforceClient.diff with moved files and capability on ... ERROR
	+	Testing PerforceClient.diff with a pending changelist ... ERROR
	+	Testing PerforceClient.parse_revision_spec with invalid specifications ... ok
	+	Testing PerforceClient.parse_revision_spec with no specified revisions ... ERROR
	+	Testing PerforceClient.parse_revision_spec with a pending changelist ... ok
	+	Testing PerforceClient.parse_revision_spec with a shelved changelist ... ok
	+	Testing PerforceClient.parse_revision_spec with a submitted changelist ... ok
	+	Testing PerforceClient.parse_revision_spec with two changelists ... ok
	+	Testing PerforceClient.get_repository_info ... ok
	+	Testing PerforceClient.get_repository_info outside client root ... ok
	+	Testing PerforceClient.scan_for_server_counter with reviewboard.url ... ok
	+	Testing PerforceClient.scan_for_server_counter with encoded reviewboard.url.http:\|\| ... ok
	+	...
	+

Diff:

Revision 2 (+6 -5)

Show changes

rbtools/clients/perforce.py

Tool: Pyflakes
Processed Files:
    rbtools/clients/perforce.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/clients/perforce.py

Summary:: Regression from 0.7.4 - Fails to post a review from Perforce client
rbt fails to post a review from Perforce client - regression from 0.7.4
Description:: ~
Fix for the ticket: https://hellosplat.com/s/beanbag/tickets/4003/
~
rbt will fail (with Python exception) to post review based on Perforce working directory for revision range specified like this:
+
+
rbt post //repo/path/...@123,@456
+
+
Note: My familiarity with the code base is poor and most likely this fix is NOT good for production. I mentioned it in the bug description. I post this review so you guys can fix it properly.
+
+
The comments section in the get_changenum method says that revisions dict comes from parse_revision_spec. So, it's likely that the real problem is in the parse_revision_spec, or in the way its return value is stored and later reused with get_changenum. I have no idea, really.
+
+
The fix I post here (as per Barret's request in the ticket response) prevents RBTools from failing so devs in my team can still use it for posting reviews.
+
+
I ran unit tests as recommended here: https://www.reviewboard.org/docs/codebase/dev/getting-started/
+ I don't know why so many of them are failing. Maybe my environment is missing some components. I also don't have all relevant SCM tools on my machine (we only use Perforce).
+
+
So far my team has great difficulties using RB with Perforce. We can't use P4 generated diffs neither in unified diff format, nor in Perforce format, see https://hellosplat.com/s/beanbag/tickets/4002/ .
+ We're trying to use lastest RBTools, but as mentioend above the tool fails, as well.

Status:: Discarded

~		Fix for the ticket: https://hellosplat.com/s/beanbag/tickets/4003/
	~	rbt will fail (with Python exception) to post review based on Perforce working directory for revision range specified like this:
	+
	+	rbt post //repo/path/...@123,@456
	+
	+	Note: My familiarity with the code base is poor and most likely this fix is NOT good for production. I mentioned it in the bug description. I post this review so you guys can fix it properly.
	+
	+	The comments section in the get_changenum method says that revisions dict comes from parse_revision_spec. So, it's likely that the real problem is in the parse_revision_spec, or in the way its return value is stored and later reused with get_changenum. I have no idea, really.
	+
	+	The fix I post here (as per Barret's request in the ticket response) prevents RBTools from failing so devs in my team can still use it for posting reviews.
	+
	+	I ran unit tests as recommended here: https://www.reviewboard.org/docs/codebase/dev/getting-started/
	+	I don't know why so many of them are failing. Maybe my environment is missing some components. I also don't have all relevant SCM tools on my machine (we only use Perforce).
	+
	+	So far my team has great difficulties using RB with Perforce. We can't use P4 generated diffs neither in unified diff format, nor in Perforce format, see https://hellosplat.com/s/beanbag/tickets/4002/ .
	+	We're trying to use lastest RBTools, but as mentioend above the tool fails, as well.