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.

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

Improve GitHub support #1487

Merged
merged 20 commits into from Jan 31, 2015
Merged

Improve GitHub support #1487

merged 20 commits into from Jan 31, 2015

Conversation

sa2ajj
Copy link
Contributor

@sa2ajj sa2ajj commented Jan 17, 2015

No description provided.

return _STATE_MAP.get(results, 'error')


def _timeDeltaToHumanReadable(start, end):
Copy link
Member

Choose a reason for hiding this comment

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

There is nothing specific to Github here.
That could be used by other status plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True.

That was my next step (first I wanted to make sure that there's a test for custom GitHub URL).

@sa2ajj
Copy link
Contributor Author

sa2ajj commented Jan 17, 2015

I still do not have test cases for the new functionality

@sa2ajj
Copy link
Contributor Author

sa2ajj commented Jan 17, 2015

And, actually, I am not sure how to do that. It seems that FakeRequest needs to be re-written (current implementation is really hackish).

@djmitche
Copy link
Member

Yes, FakeRequest is pretty bogus. Twisted's testing utilities may have a useful alternative.

@sa2ajj
Copy link
Contributor Author

sa2ajj commented Jan 24, 2015

@djmitche I looked at what Twisted offers and it's not much.

Maybe you could suggest what test cases would be "enough"? I really want this PR to land in eight before next release.

@djmitche
Copy link
Member

This looks like a good set of tests now. Coverage is only missing for the various error-handling bits (raise ValueError..) and for the signature verification.

@sa2ajj
Copy link
Contributor Author

sa2ajj commented Jan 25, 2015

  • signature verification
  • error cases

@sa2ajj sa2ajj changed the title [WIP/DO NOT MERGE] Improve GitHub support Improve GitHub support Jan 26, 2015
@sa2ajj
Copy link
Contributor Author

sa2ajj commented Jan 26, 2015

(Needs port to master)

@djmitche
Copy link
Member

I still get a few pep8 changes and some test failures from validate.sh.

[ERROR]
Traceback (most recent call last):
  File "/home/dustin/code/buildbot/t/buildbot/master/buildbot/test/unit/test_status_github.py", line 70, in setUp
    token='token', repoOwner='owner', repoName='name')
  File "/home/dustin/code/buildbot/t/buildbot/master/buildbot/status/github.py", line 77, in __init__
    self._github = GitHubAPI(oauth2_token=token, baseURL=baseURL)
exceptions.TypeError: __init__() got an unexpected keyword argument 'baseURL'

buildbot.test.unit.test_status_github.TestGitHubStatus.test_buildFinished_failure
buildbot.test.unit.test_status_github.TestGitHubStatus.test_buildFinished_success
buildbot.test.unit.test_status_github.TestGitHubStatus.test_buildStarted_failure
buildbot.test.unit.test_status_github.TestGitHubStatus.test_buildStarted_success
buildbot.test.unit.test_status_github.TestGitHubStatus.test_builderAdded
buildbot.test.unit.test_status_github.TestGitHubStatus.test_custom_github_url
buildbot.test.unit.test_status_github.TestGitHubStatus.test_getGitHubRepoProperties_ok
buildbot.test.unit.test_status_github.TestGitHubStatus.test_getGitHubRepoProperties_skip_no_name
buildbot.test.unit.test_status_github.TestGitHubStatus.test_getGitHubRepoProperties_skip_no_owner
buildbot.test.unit.test_status_github.TestGitHubStatus.test_getGitHubRepoProperties_skip_no_sha
buildbot.test.unit.test_status_github.TestGitHubStatus.test_getGitHubState
buildbot.test.unit.test_status_github.TestGitHubStatus.test_initialization_required_arguments
buildbot.test.unit.test_status_github.TestGitHubStatus.test_sendFinishStatus_failure
buildbot.test.unit.test_status_github.TestGitHubStatus.test_sendFinishStatus_no_properties
buildbot.test.unit.test_status_github.TestGitHubStatus.test_sendFinishStatus_ok
buildbot.test.unit.test_status_github.TestGitHubStatus.test_sendGitHubStatus_error
buildbot.test.unit.test_status_github.TestGitHubStatus.test_sendGitHubStatus_success
buildbot.test.unit.test_status_github.TestGitHubStatus.test_sendStartStatus_failure
buildbot.test.unit.test_status_github.TestGitHubStatus.test_sendStartStatus_no_properties
buildbot.test.unit.test_status_github.TestGitHubStatus.test_sendStartStatus_ok
buildbot.test.unit.test_status_github.TestGitHubStatus.test_startService

@sa2ajj
Copy link
Contributor Author

sa2ajj commented Jan 26, 2015

What is the exact command you are running?

@djmitche
Copy link
Member

sh common/validate.sh HEAD^

@djmitche
Copy link
Member

and, I did re-run pip install -e master in case of setup.py differences between master and eight.

@sa2ajj
Copy link
Contributor Author

sa2ajj commented Jan 27, 2015

Right. I believe you need to update txgithub package (since it's OK to run Buildbot without it, txgithub package does not have a proper versioned dependency).

@djmitche
Copy link
Member

OK, looks good to me! Sorry to delay this so much..

djmitche added a commit to djmitche/buildbot that referenced this pull request Jan 31, 2015
kchang and others added 7 commits January 31, 2015 13:26
* remove unused self._token
* simplify expressions for sha, startDescription, endDescription
* re-format addErrback
* change import for datetime to simplify its usage
* _getGitHubState
* _timeDeltaToHumanReadable
sa2ajj pushed a commit that referenced this pull request Jan 31, 2015
@sa2ajj sa2ajj merged commit f44bf2d into buildbot:eight Jan 31, 2015
@sa2ajj sa2ajj deleted the improve-github-support branch January 31, 2015 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants