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

Feature/refactor can modify vote to validator service #334

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

szirbucz
Copy link
Contributor

No description provided.

@szirbucz szirbucz requested a review from magwas July 15, 2019 06:36
Copy link
Member

@magwas magwas left a comment

Choose a reason for hiding this comment

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

Again, comments are just on some representative places, and handling of test data should be figured out.


@TestedFeature("Manage votes")
@TestedOperation("delete choice")
@RunWith(MockitoJUnitRunner.class)
public class ChoiceDeleteAdminKeyIsUserTest extends ChoiceTestBase {

private static final String NOT_SAME_USER_MESSAGE =
"The adminKey is \"user\" but the user is not same with that user who added the choice.";
private static final String CAN_ADDIN_IS_FALSE_MESSAGE =
Copy link
Member

Choose a reason for hiding this comment

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

Aren't those strings are constants in the production code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are. If you mean, we should use constants of the production code, I suggest to forget it.
Let's imagine the situation, when someone accidentally removes some words from this string or an unlucky regex overwrites it. We wouldn't realize it, since we compare this constant to itself.

voteAdminInfo,
choiceToDelete.getId()
)
).assertMessageIs("The adminKey is \"user\" but canAddin is false.");
Copy link
Member

Choose a reason for hiding this comment

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

Once a java constant, once an inline constant.

assertThrows(() -> choiceService.deleteChoice(new VoteAdminInfo(vote.getId(), USER),
choiceToDelete.getId())).assertMessageIs("The adminKey is \"user\" but canAddin is false.");
final VoteAdminInfo voteAdminInfo = new VoteAdminInfo(vote.getId(), USER);
doThrow(new ReportedException(CAN_ADDIN_IS_FALSE_MESSAGE))
Copy link
Member

Choose a reason for hiding this comment

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

Service stub, should be put into a stub provider.

public class ChoiceModificationValidatorServiceTest
extends ChoiceValidatorBaseTest {

private static final String MODIFICATION = "modification";
Copy link
Member

Choose a reason for hiding this comment

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

Production code constant

@@ -1,7 +1,8 @@
package org.rulez.demokracia.pdengine.choice;

import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.*;
import static org.mockito.Mockito.doThrow;
Copy link
Member

Choose a reason for hiding this comment

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

This is a sign that there are stubs which do not belong here.

@@ -17,6 +18,11 @@
@RunWith(MockitoJUnitRunner.class)
public class ChoiceModifyValidationTest extends ChoiceTestBase {

private static final String DIFFERENT_USER_MESSAGE =
Copy link
Member

Choose a reason for hiding this comment

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

constant duplication

@@ -34,37 +40,50 @@ public void setUp() {
@TestedBehaviour(VALIDATES_INPUTS)
@Test
public void invalid_voteId_is_rejected() {
doThrow(new ReportedException("illegal voteId", invalidvoteId)).when(voteService)
doThrow(new ReportedException("illegal voteId", invalidvoteId))
Copy link
Member

Choose a reason for hiding this comment

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

stub

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