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.

Sign up for GitHub

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

Jump to bottom

Substitute mockito with mockk and assertj with kluent #177

Merged
merged 43 commits into from
Jul 5, 2020
Merged

Substitute mockito with mockk and assertj with kluent #177

merged 43 commits into from
Jul 5, 2020

Conversation

ursjoss
Copy link
Owner

@ursjoss ursjoss commented May 2, 2020 •
edited
Loading

  • Convert remaining tests to kotlin
    • public-web
    • core-web
  • Remove dependencies:
    • mockito
    • mockito-kotlin
    • assertj
  • Manage performance - check effect of
    • build scans from master and this branch to compare
    • clearing mocks
    • favour directly instantiating in code over annotations

@ursjoss ursjoss self-assigned this May 2, 2020
@ursjoss ursjoss added the chore label May 4, 2020
@ursjoss ursjoss force-pushed the chore/remove_mockito_assertj branch from 314ddbf to eaea5e0 Compare May 5, 2020 06:03
@ursjoss
Copy link
Owner Author

ursjoss commented May 5, 2020 •
edited
Loading

switching to my probably naive approach of substituting mockito with mock and assertj with kluent resulted in a build time for gradlew check from roughly 20 minutes to 100 or more minutes. :-( I will have to assess the root cause and find some counter measures before merging...

@ursjoss
Copy link
Owner Author

ursjoss commented Jun 11, 2020

Build scan from master:
image

From this branch:
image

@ursjoss ursjoss force-pushed the chore/remove_mockito_assertj branch from 60b96ce to e98f02d Compare June 12, 2020 14:48
@ursjoss
Copy link
Owner Author

ursjoss commented Jun 13, 2020

After implementing the clearing of all mocks, the total build time went even further up, from roughly 120 minutes to ~230 minutes :-(

@ursjoss ursjoss changed the title Chore/remove mockito assertj Substitute mockito with mockk and assertj with kluent Jun 21, 2020
@ursjoss ursjoss force-pushed the chore/remove_mockito_assertj branch from 4dd2430 to dae436c Compare June 28, 2020 17:19
ursjoss added 18 commits June 29, 2020 06:40
@ursjoss
Avoid relaxing in some tests, avoid mocking if possible
98427e8
@ursjoss
Fix test fixtures
ffd3397
@ursjoss
Raise MaxMetaspace to 3G
b878079
@ursjoss
Increase maxHeapSize for tests
3afbe1b
@ursjoss
Merge test task configuration
0f84c84
@ursjoss
Raise memory for kotlin compiler
38840f6
@ursjoss
Temporarily disable the slowest tests
0ac9401
@ursjoss
Disable the slowest tests from July 10
a2cf86a
@ursjoss
Reactivate ResultPanelInEditModeTest
fb2fb61
@ursjoss
Fix mocking of AjaxRequestTarget
bebfe23 
  * unmockAll()
  * dont mock relaxedly
@ursjoss
Fix spring-boot-configuration-processor setup
c60b859
@ursjoss
Cleanup tests
f128713 
  * Avoid using mocks where possible
  * unmockAll
  * tester.destroy
  * Use AjaxTargetRequestSpy instead of mocking AjaxTargetRequest
@ursjoss
Use improved TestAjaxRequestHandler to considerably speed up the tests
49addb2 
Use LinkedHashSet instead of LinkedList to store the instances of
IListener. This improves the time complexity in method addListener
for the contains method from O(n) to O(1). This brings down the time
spent for running the `gradlew check` command for the entire project
from several hours to 20 minutes again (with mockk - mockito was not
affected that much).
@ursjoss
Use HashSet in TestAjaxRequestHandler - trading speed for maintaining…
a5e946b 
… insertion order
@ursjoss
Remove unmockkAll
a70ecd5
@ursjoss
Revert "Use HashSet in TestAjaxRequestHandler - trading speed for mai…
143fbd3 
…ntaining insertion order"

This reverts commit 2258b71.
@ursjoss
Revert "Raise memory for kotlin compiler"
b22f981 
This reverts commit 38840f6.
@ursjoss
Revert "Raise MaxMetaspace to 3G"
0de4cd6 
This reverts commit b878079.
@ursjoss ursjoss force-pushed the chore/remove_mockito_assertj branch from 1a7205b to 0de4cd6 Compare July 5, 2020 11:44
@ursjoss
Copy link
Owner Author

ursjoss commented Jul 5, 2020

The wicket AjaxRequestHandler implementation uses a LinkedList to store listeners. When adding new listeners (method addListener), to prevent duplicates, it uses the method contains to verify the list does not yet contain the event. This scales very badly. In order to work around this, commit 49addb2 introduces a modified copy TestAjaxRequestHandler that replaces the LinkedList with LinkedHashSet that allows calling contains with constant time complexity. This brings down the total build time from 2-4 hours to roughly 20-30 minutes. It is yet unclear why this was no problem with mockito but surfaced only with the transition to mock. This will potentially result in pull requests for wicket and/or mock. But I can merge this PR now.

@ursjoss ursjoss merged commit 356290e into master Jul 5, 2020
@ursjoss ursjoss deleted the chore/remove_mockito_assertj branch July 5, 2020 12:24
@ursjoss ursjoss mentioned this pull request Jul 5, 2020
Merged
@ursjoss
Copy link
Owner Author

ursjoss commented Jul 5, 2020

PR in wicket: apache/wicket#438

@ursjoss ursjoss mentioned this pull request Jul 11, 2020
Open
@ursjoss ursjoss mentioned this pull request Jul 19, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ursjoss ursjoss

Labels
chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@ursjoss