Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix problem with Campfire hook notifying on start. #36

Merged
merged 4 commits into from Mar 29, 2012

Conversation

Projects
None yet
3 participants
Owner

roidrage commented Mar 28, 2012

Also start on a more higher-level spec for the notifications system in general. A work in progress. This fixes the problems caused by #34.

@roidrage roidrage Fix problem with Campfire hook notifying on start.
Also start on a more higher-level spec for the notifications
system in general. A work in progress.
f3e268c

@dmathieu dmathieu and 1 other commented on an outdated diff Mar 28, 2012

lib/travis/notifications/handler/campfire.rb
@@ -35,6 +36,7 @@ def build_url(build)
end
def notify(event, object, *args)
+ puts 'boom'
@dmathieu

dmathieu Mar 28, 2012

Contributor

That's not left on purpose right ? :)

@roidrage

roidrage Mar 28, 2012

Owner

Haha, no! :)

Contributor

dmathieu commented Mar 28, 2012

Thank you for the global notifications specs. That's awesome ❤️

Contributor

dmathieu commented Mar 28, 2012

I believe it's possible to add a test checking for this regression.

Owner

roidrage commented Mar 29, 2012

For which regression? For "boom"? ;)

Contributor

dmathieu commented Mar 29, 2012

No, for the regex which was missing.

Owner

roidrage commented Mar 29, 2012

Is there a test missing for this? I may have overlooked something.

Contributor

dmathieu commented Mar 29, 2012

Oh I see, I overlooked the notifications_spec file.
Perhaps this is not the best place to put webhook and campfire notifications ?

There are campfire_spec and webhook_spec files.

@roidrage roidrage Instrument notifications with metrics.
Add first stab at a test suite for the subscription class.
676a4ee
Owner

roidrage commented Mar 29, 2012

The reason I added this more higher-level test is because the campfire spec can't really test if an event is not triggered when the event doesn't match, that happens when a subscription is triggered. What it could do is make sure EVENTS only contains build:finished. That's certainly up for arguments, but I also started the test for notifications in general, because the whole system is lacking more higher level means of testing.

@roidrage roidrage Add explicit tests about what the EVENTS constants.
More explicit tests for the correct event subscriptions.
3b17b67
Owner

roidrage commented Mar 29, 2012

Okay, added some more explicit tests for the EVENTS constant. This should make sure something like this doesn't come up on several levels :)

Contributor

dmathieu commented Mar 29, 2012

Thank you :)

@dmathieu dmathieu added a commit that referenced this pull request Mar 29, 2012

@dmathieu dmathieu Merge pull request #36 from travis-ci/fix-campfire-notifications
Fix problem with Campfire hook notifying on start.
3e5b061

@dmathieu dmathieu merged commit 3e5b061 into master Mar 29, 2012

Owner

joshk commented Mar 29, 2012

Hey @dmathieu,

Was this fix tested on Staging before being merged in?

We are trying to be a little more cautious about merging in PRs, the main goal is to first test on staging and then merge in.

I think it is also good to comment on the PR asking if it is ready for merging and ready to be tested on staging, using the PR more as a discussion then just a way to contribute code.

Let me know if you have any questions/comments about this.

Thanks a bundle,

Josh

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