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

P4 master step 2012 #596

Merged
merged 25 commits into from Apr 5, 2013

Conversation

Projects
None yet
4 participants
@bdbaddog
Member

bdbaddog commented Dec 26, 2012

This is the initial whack at a perforce master side source step.
Feed back welcome.

I'll be working on adding some tests over the next week or so.

@jaredgrubb

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@jaredgrubb

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@jaredgrubb

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@jaredgrubb

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@jaredgrubb

This comment has been minimized.

Show comment
Hide comment
@jaredgrubb

jaredgrubb Dec 28, 2012

Member

I looked at this briefly, as I've never used Perforce, but a few comments are included.

Any reason all your parameters are prefixed by "p4" (like "p4branch")? The other source steps use words like "branch" and "username", not like "svnUser". Unless there's a reason to use 'p4Foo', it's probably better to use a generic word (especially where there's a similar parameter used for other Source types).

Member

jaredgrubb commented Dec 28, 2012

I looked at this briefly, as I've never used Perforce, but a few comments are included.

Any reason all your parameters are prefixed by "p4" (like "p4branch")? The other source steps use words like "branch" and "username", not like "svnUser". Unless there's a reason to use 'p4Foo', it's probably better to use a generic word (especially where there's a similar parameter used for other Source types).

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Dec 29, 2012

Member

Yay for docs, but this needs a substantial set of tests, too :)

Member

djmitche commented Dec 29, 2012

Yay for docs, but this needs a substantial set of tests, too :)

@bdbaddog

This comment has been minimized.

Show comment
Hide comment
@bdbaddog

bdbaddog Jan 6, 2013

Member

djmitche:

Current p4 source step has 0 tests.. (Yes. I know that doesn't make it right. ;)
Though there are some tests for p4poller.
What would be a reasonable set of tests for this?
Would the SVN master side source step be a reasonable example to base test from?

Member

bdbaddog commented Jan 6, 2013

djmitche:

Current p4 source step has 0 tests.. (Yes. I know that doesn't make it right. ;)
Though there are some tests for p4poller.
What would be a reasonable set of tests for this?
Would the SVN master side source step be a reasonable example to base test from?

@tomprince

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@tomprince

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Jan 9, 2013

Member

Yes, the svn tests (or any of the master-side steps) provide a good model for the tests. I know there aren't any tests for P4 right now .. it makes me 😦

Member

djmitche commented Jan 9, 2013

Yes, the svn tests (or any of the master-side steps) provide a good model for the tests. I know there aren't any tests for P4 right now .. it makes me 😦

@jaredgrubb

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@bdbaddog

This comment has been minimized.

Show comment
Hide comment
@bdbaddog

bdbaddog Mar 25, 2013

Member

O.k. 18 tests now.
Made some significant changes from the initial pull.

Please review as the current code is hopefully acceptable.

Regarding the use of p4FOO, that's the standard for working with perforce, you can specify all those as environment variables P4USER,etc. Also you can provide a dotfile named whatever the value of P4CONFIG's environment variable and specify the values P4USER=bdbaddog in such a file.

Member

bdbaddog commented Mar 25, 2013

O.k. 18 tests now.
Made some significant changes from the initial pull.

Please review as the current code is hopefully acceptable.

Regarding the use of p4FOO, that's the standard for working with perforce, you can specify all those as environment variables P4USER,etc. Also you can provide a dotfile named whatever the value of P4CONFIG's environment variable and specify the values P4USER=bdbaddog in such a file.

@jaredgrubb

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@jaredgrubb

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@jaredgrubb

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Mar 30, 2013

Member

From a quick paging through the source, there's still at least one commented-out test case, and lots of lines have trailing whitespace.

Member

djmitche commented Mar 30, 2013

From a quick paging through the source, there's still at least one commented-out test case, and lots of lines have trailing whitespace.

@djmitche

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@djmitche

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@djmitche

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@djmitche

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@djmitche

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@djmitche

View changes

Show outdated Hide outdated master/buildbot/steps/source/p4.py Outdated
@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Mar 30, 2013

Member

Looking good!

Member

djmitche commented Mar 30, 2013

Looking good!

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Apr 5, 2013

Member

Don't forget to comment when you add commits to a pull request, so we know to look!

Member

djmitche commented Apr 5, 2013

Don't forget to comment when you add commits to a pull request, so we know to look!

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Apr 5, 2013

Member

Looks good. I'll clean up some formatting while merging.

Member

djmitche commented Apr 5, 2013

Looks good. I'll clean up some formatting while merging.

@djmitche

This comment has been minimized.

Show comment
Hide comment
@djmitche

djmitche Apr 5, 2013

Member

Hm, it's a little worse than that now that I look. The formatting stuff is:

  • remove the docstrings from the P4 class, since they're redundant to the docs in the manual
  • strip trailing whitespace from cfg-buildsteps.rst
  • remove the # The step which would should issue the commands in et seq. comments from the tests
  • find a way to not have such long lines in the test file. textwrap.dedent is a great tool for this.

While I was cleaning up the last of those, though, I noticed that for many of the tests, the expected behavior is exactly the same as for other arguments. In particluar, most calls to self._full pass exactly the same expectedStdin. It looks like this is because the parameters you pass to the constructor in those tests are equivalent to the defaults, but the end result is that the tests don't very convincingly indicate that those parameters actually have the desired effect. That's the part where I'd like you to have another look - if I make the changes that "seem" right, I may end up just writing tests to guarantee incorrect behavior.

Member

djmitche commented Apr 5, 2013

Hm, it's a little worse than that now that I look. The formatting stuff is:

  • remove the docstrings from the P4 class, since they're redundant to the docs in the manual
  • strip trailing whitespace from cfg-buildsteps.rst
  • remove the # The step which would should issue the commands in et seq. comments from the tests
  • find a way to not have such long lines in the test file. textwrap.dedent is a great tool for this.

While I was cleaning up the last of those, though, I noticed that for many of the tests, the expected behavior is exactly the same as for other arguments. In particluar, most calls to self._full pass exactly the same expectedStdin. It looks like this is because the parameters you pass to the constructor in those tests are equivalent to the defaults, but the end result is that the tests don't very convincingly indicate that those parameters actually have the desired effect. That's the part where I'd like you to have another look - if I make the changes that "seem" right, I may end up just writing tests to guarantee incorrect behavior.

@@ -415,6 +415,7 @@ def __init__(self, fmtstring, *args, **kwargs):
self.interpolations = {}
self._parse(fmtstring)
# TODO: add case below for when there's no args or kwargs..

This comment has been minimized.

@djmitche

djmitche Apr 5, 2013

Member

I just took care of this.

@djmitche

djmitche Apr 5, 2013

Member

I just took care of this.

djmitche and others added some commits Apr 5, 2013

used textwrap.dedent() to get rid of long lines per pull feedback. Ad…
…dressed djmitche's comments regarding full tests not having any variation in the expected generated clientspec by varying the parameters
@bdbaddog

This comment has been minimized.

Show comment
Hide comment
@bdbaddog

bdbaddog Apr 5, 2013

Member

Pushed changes to address latest feedback.

Member

bdbaddog commented Apr 5, 2013

Pushed changes to address latest feedback.

@djmitche djmitche merged commit 5d7e518 into buildbot:master Apr 5, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment