Upgrade to new dependency mockito-testng #10647

skabashnyuk · 2018-08-03T10:47:38Z

What does this PR do?

Upgrade to new dependency mockito-testng
Fixied issue in code related to this changes in mockito

thenThrow(Class) should only throw checked exceptions that match the signature of the stubbed method (thenThrow(Class) should only throw checked exceptions that match the signature of the stubbed method mockito/mockito#1155)
Cleaner code for Mockito users by better integration with static code checkers, more @CheckReturnValue (Cleaner code for Mockito users by better integration with static code checkers, more @CheckReturnValue mockito/mockito#1270)
Add @CheckReturnValue to stubbing/verification methods (Add @CheckReturnValue to stubbing/verification methods mockito/mockito#1228)
Removed usused stubs from test classes (stubbed methods that would never be invoked during test executions).
In some cases, stubs would be counted as unused, when one is declared for multiple tests in @BeforeMethod, and at least one of the tests ends up not invoking the stubbed method. Usual solution to that is disabling strict checking for that stub using static method lenient(), or moving/removing stubs.
In case when same method would be stubbed with when().then() (but with different arguments), an exception will occur about unnecessary duplicated. To fix that, stubbing were reworked into doReturn().when().
In other cases, when stubbing is intentionally declared, but not invoked, a lenient() method is used.

Merge state

What issues does this PR fix or reference?

mockito/mockito#962
eclipse-che/che-parent#77
#10617

Upgrade to official mockito-testng artifact

Release Notes

Upgrade to official mockito-testng artifact

Docs PR

n/a

garagatyi · 2018-08-06T12:11:48Z

...t/che-core-git-impl-jgit/src/test/java/org/eclipse/che/git/impl/jgit/JGitConnectionTest.java

@@ -192,7 +191,8 @@ public void shouldDoNothingWhenTransportHttpReceived() throws Exception {
     * with  help real children {@link TransportHttp}, which returns real not null
     * value. And then we can create mock {@link TransportHttp}.
     */
-    mock(Transport.class);


Is this change needed in this PR?

I guess yes. AFAIK @AndrienkoAleksandr was trying to explain why it needed in a comment about. New mockito just checking that mock is assigned to some variable and we can't just have mock in a vacuum.

garagatyi · 2018-08-06T12:13:16Z

...he/workspace/infrastructure/openshift/multiuser/oauth/IdentityProviderConfigBuilderTest.java

@@ -225,7 +225,8 @@ public void testRethrowOnInvalidTokenBadRequestException() throws Exception {

  @Test(expectedExceptions = {InfrastructureException.class})
  public void testRethrowOnAnyException() throws Exception {
-    when(keycloakServiceClient.getIdentityProviderToken(anyString())).thenThrow(Exception.class);


Can you elaborate on why we need this change? I don't see any connection with the purpose of the PR.

With new mockito code should only throw checked exceptions that match the signature of the stubbed. In this case, that was not true.

garagatyi · 2018-08-06T12:13:36Z

.../java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesInternalRuntimeTest.java

@@ -357,7 +357,7 @@ public void stopsWaitingAllMachineStartWhenOneMachineStartFailed() throws Except
    final ImmutableMap<String, Pod> allPods =
        ImmutableMap.of(WORKSPACE_POD_NAME, mockPod(ImmutableList.of(container1, container2)));
    when(k8sEnv.getPods()).thenReturn(allPods);
-    doThrow(InfrastructureException.class).when(bootstrapper).bootstrapAsync();
+    doThrow(IllegalStateException.class).when(bootstrapper).bootstrapAsync();


Can you elaborate on why we need this change?

With new mockito code should only throw checked exceptions that match the signature of the stubbed. In this case, that was not true.

garagatyi · 2018-08-06T12:13:43Z

.../java/org/eclipse/che/workspace/infrastructure/kubernetes/KubernetesInternalRuntimeTest.java

@@ -378,7 +378,7 @@ public void throwsInfrastructureExceptionWhenErrorOccursAndCleanupFailed() throw
    doNothing().doThrow(InfrastructureException.class).when(namespace).cleanUp();
    when(k8sEnv.getServices()).thenReturn(singletonMap("testService", mock(Service.class)));
    when(services.create(any())).thenThrow(new InfrastructureException("service creation failed"));
-    doThrow(InfrastructureException.class).when(namespace).services();


Can you elaborate on why we need this change? I don't see any connection with the purpose of the PR.

same as two comments above. With new mockito code should only throw checked exceptions that match the signature of the stubbed. In this case, that was not true.

garagatyi · 2018-08-06T12:22:43Z

@skabashnyuk thank you for the explanation!

musienko-maxim · 2018-09-26T09:13:31Z

ci-test

riuvshin · 2018-09-26T09:21:46Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:10647
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

mkuznyetsov · 2018-10-08T11:43:46Z

ci-test

riuvshin · 2018-10-08T11:53:12Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:10647
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

mkuznyetsov · 2018-10-08T11:53:56Z

ci-test

riuvshin · 2018-10-08T11:59:24Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:10647
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

mkuznyetsov · 2018-10-08T12:02:43Z

ci-test

riuvshin · 2018-10-08T14:46:14Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:10647
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

mkuznyetsov · 2018-10-09T11:34:26Z

MockitoTestNG runs with enabled Strict Stubbing rules, so tests were adjusted to that:

Removed usused stubs from test classes (stubbed methods that would never be invoked during test executions).
In some cases, stubs would be counted as unused, when one is declared for multiple tests in @BeforeMethod, and at least one of the tests ends up not invoking the stubbed method. Usual solution to that is disabling strict checking for that stub using static method lenient().
In case when same method would be stubbed with when().then() (but with different arguments), an exception will occur about unnecessary duplicated. To fix that, stubbing were reworked into doReturn().when().
In other cases, when stubbing is intentionally declared, but not invoked, a lenient() method is used.

More info on how Strict Stubbing checking workes at:
https://static.javadoc.io/org.mockito/mockito-core/2.7.1/org/mockito/exceptions/misusing/PotentialStubbingProblem.html

mkuznyetsov · 2018-10-10T00:01:03Z

ci-test

riuvshin · 2018-10-10T02:50:31Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:10647
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

skabashnyuk added 2 commits August 2, 2018 15:49

Upgrade to new dependency mockito-testng

c7f4062

Throw only exceptions allowed by method signature

f4be5df

skabashnyuk requested review from dkulieshov, dmytro-ndp, garagatyi, musienko-maxim, Ohrimenko1988, riuvshin, sleshchenko, tolusha and vparfonov as code owners August 3, 2018 10:47

garagatyi reviewed Aug 6, 2018

View reviewed changes

garagatyi approved these changes Aug 6, 2018

View reviewed changes

benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Aug 9, 2018

mkuznyetsov added 2 commits September 24, 2018 12:37

Merge branch 'master' of github.com:eclipse/che into che10617

6d14132

Fix new mockito artifact name

7b8d61e

skabashnyuk requested a review from mshaposhnik as a code owner September 24, 2018 09:48

AndrienkoAleksandr approved these changes Sep 24, 2018

View reviewed changes

mkuznyetsov added 6 commits October 3, 2018 11:06

Merge branch 'master' of github.com:eclipse/che into che10617

1091efa

Adapt to new Mockito TestNG version

6400fa4

fixup! Adapt to new Mockito TestNG version

d244938

merge with master

c2e02ab

Fix mock strictness

0654c56

Add maven repository with text artifact for CI tests

3c59824

remove test repository

8918c69

Fix Selenium Test

1cabf96

remove odd comment

1f8305b

mkuznyetsov merged commit 65bd65e into master Oct 10, 2018

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Oct 10, 2018

benoitf added this to the 6.13.0 milestone Oct 10, 2018

nickboldt pushed a commit to nickboldt/che that referenced this pull request Oct 15, 2018

Upgrade to new dependency mockito-testng (eclipse-che#10647)

8706240

nickboldt pushed a commit to nickboldt/che that referenced this pull request Oct 15, 2018

Upgrade to new dependency mockito-testng (eclipse-che#10647)

a5644ab

skabashnyuk deleted the che10617 branch November 17, 2018 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to new dependency mockito-testng #10647

Upgrade to new dependency mockito-testng #10647

skabashnyuk commented Aug 3, 2018 •

edited by mkuznyetsov

Loading

garagatyi Aug 6, 2018

skabashnyuk Aug 6, 2018

garagatyi Aug 6, 2018

skabashnyuk Aug 6, 2018

garagatyi Aug 6, 2018

skabashnyuk Aug 6, 2018

garagatyi Aug 6, 2018

skabashnyuk Aug 6, 2018 •

edited

Loading

garagatyi commented Aug 6, 2018

musienko-maxim commented Sep 26, 2018

riuvshin commented Sep 26, 2018

mkuznyetsov commented Oct 8, 2018

riuvshin commented Oct 8, 2018

mkuznyetsov commented Oct 8, 2018

riuvshin commented Oct 8, 2018

mkuznyetsov commented Oct 8, 2018

riuvshin commented Oct 8, 2018

mkuznyetsov commented Oct 9, 2018

mkuznyetsov commented Oct 10, 2018

riuvshin commented Oct 10, 2018

Upgrade to new dependency mockito-testng #10647

Upgrade to new dependency mockito-testng #10647

Conversation

skabashnyuk commented Aug 3, 2018 • edited by mkuznyetsov Loading

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

garagatyi Aug 6, 2018

Choose a reason for hiding this comment

skabashnyuk Aug 6, 2018

Choose a reason for hiding this comment

garagatyi Aug 6, 2018

Choose a reason for hiding this comment

skabashnyuk Aug 6, 2018

Choose a reason for hiding this comment

garagatyi Aug 6, 2018

Choose a reason for hiding this comment

skabashnyuk Aug 6, 2018

Choose a reason for hiding this comment

garagatyi Aug 6, 2018

Choose a reason for hiding this comment

skabashnyuk Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

garagatyi commented Aug 6, 2018

musienko-maxim commented Sep 26, 2018

riuvshin commented Sep 26, 2018

mkuznyetsov commented Oct 8, 2018

riuvshin commented Oct 8, 2018

mkuznyetsov commented Oct 8, 2018

riuvshin commented Oct 8, 2018

mkuznyetsov commented Oct 8, 2018

riuvshin commented Oct 8, 2018

mkuznyetsov commented Oct 9, 2018

mkuznyetsov commented Oct 10, 2018

riuvshin commented Oct 10, 2018

skabashnyuk commented Aug 3, 2018 •

edited by mkuznyetsov

Loading

skabashnyuk Aug 6, 2018 •

edited

Loading