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

tests for web notification consumer #404

Merged
merged 1 commit into from Aug 28, 2019

Conversation

@aquibbaig
Copy link
Collaborator

aquibbaig commented Aug 17, 2019

Tried writing some tests for the web notification consumer. @imphil @agathver Please suggest if I have missed something on this.

Since I found that protected/private methods should not be tested, I did some refactoring to the existing code itself to test out the handle() function

@aquibbaig aquibbaig requested a review from imphil Aug 17, 2019
@aquibbaig aquibbaig added the gsoc label Aug 17, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:notification-testing branch 3 times, most recently from 5750a07 to b96ca7a Aug 18, 2019
@aquibbaig aquibbaig added this to Review in progress in GSoC 2019 Aug 19, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:notification-testing branch from b96ca7a to 7e107bc Aug 19, 2019
*
* Tests if a notification is added to database
*/
public function test_if_notification_is_added()

This comment has been minimized.

Copy link
@imphil

imphil Aug 21, 2019

Contributor

use lowerCamelCase() for function names.

->getMock();
$webNotificationConsumer = new WebNotificationConsumer($mockLogger, $mockNotificationManager);
$webNotificationConsumer->setNotification($this->notification);

This comment has been minimized.

Copy link
@imphil

imphil Aug 21, 2019

Contributor

That doesn't test what is really happening. What you should do instead is run the execute() function with a serialized notification object.

$webNotificationConsumer->setNotification($this->notification);
// Test if handle() doesn't requeue notifications in RabbitMQ
$this->assertEquals(true, $webNotificationConsumer->handle());

This comment has been minimized.

Copy link
@imphil

imphil Aug 21, 2019

Contributor

Don't put the function call into the assert

$mockNotificationManager->expects($this->once())
->method('addNotification');
$webNotificationConsumer->handle();

This comment has been minimized.

Copy link
@imphil

imphil Aug 21, 2019

Contributor

What's that for? You already called handle() before.

}
}
?>

This comment has been minimized.

Copy link
@imphil

imphil Aug 21, 2019

Contributor

Leave that out.

$webNotificationConsumer->handle();
}
}

This comment has been minimized.

Copy link
@imphil

imphil Aug 21, 2019

Contributor

The test_if_notification_is_added() test is incomplete, it doesn't explore all possible paths the function can take.

  • What happens if shouldHandle() returns true or false? How can you test both cases?
  • In which cases returns handle() a 0 or non-zero return value? What can do you to test both cases?

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Aug 21, 2019

Author Collaborator

@imphil For testing both shouldHandle and handle, we need to set them as public first?

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Aug 21, 2019

Author Collaborator

Else we need to have a public method that calls them, what's the better option?

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Aug 22, 2019

Author Collaborator

@imphil I just wrote some of the possible tests, please have a look at it

@aquibbaig aquibbaig force-pushed the aquibbaig:notification-testing branch 3 times, most recently from 1181b94 to 2150efa Aug 21, 2019
Copy link
Contributor

imphil left a comment

Looks better, I've added some small nits and then that's ready to go.

@aquibbaig aquibbaig force-pushed the aquibbaig:notification-testing branch from 2150efa to a2d76d3 Aug 26, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 26, 2019

@imphil Fixed!

@imphil
imphil approved these changes Aug 28, 2019
GSoC 2019 automation moved this from Review in progress to Reviewer approved Aug 28, 2019
@imphil imphil merged commit 517d549 into librecores:master Aug 28, 2019
GSoC 2019 automation moved this from Reviewer approved to Done Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
GSoC 2019
  
Done
2 participants
You can’t perform that action at this time.