Skip to content

proposed fix for failing tests in MasterRegistrationTest. #3

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

Merged
merged 1 commit into from
Apr 10, 2019

Conversation

rjcausarano
Copy link
Owner

Waits for all signals to be delivered before shutting down listeners in class DefaultPublisher.

@rjcausarano rjcausarano requested a review from jubeira April 9, 2019 18:38
@rjcausarano
Copy link
Owner Author

Hey @jubeira, this might be what we were looking for. I went ahead and merged the previous PR that you reviewed and aproved so I don't understand why that commit is appearing as a change in this PR?

Also I'm pretty sure that there's a better way of handling the caught exception in the last commit. I'll make sure to fix that after your review and squash if necessary.

@rjcausarano rjcausarano force-pushed the master-registration-tests branch 2 times, most recently from 41ef4ee to 16b075d Compare April 9, 2019 20:39
@rjcausarano
Copy link
Owner Author

Alright this last commit does the following:

  • log.error() in catch block
  • shutdown latch uses passed timeout
  • adds latch.countdown() to onMasterUnregistrationFailure()
  • adds new testUnregisterPublisherFailure() test in class MasterRegistrationTest

Copy link
Collaborator

@jubeira jubeira left a comment

Choose a reason for hiding this comment

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

Great job, thanks @rjcausarano!
I've left a couple of minor comments; please address them and squash everything into one commit before sending the PR upstream.

…signals to be delivered before shutting down listeners.

Creates new testUnregisterPublisherFailure() test in class MasterRegistrationTest.
@rjcausarano rjcausarano force-pushed the master-registration-tests branch from 16b075d to 1a3fbd2 Compare April 10, 2019 13:07
@rjcausarano rjcausarano requested a review from jubeira April 10, 2019 13:08
Copy link
Collaborator

@jubeira jubeira left a comment

Choose a reason for hiding this comment

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

Awesome; LGTM! I think this can be sent upstream like this.

@rjcausarano rjcausarano merged commit 928e14c into kinetic Apr 10, 2019
@jubeira jubeira deleted the master-registration-tests branch April 10, 2019 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants