Skip to content

Implement custom rule to check spek test discovery performance issues #2954

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
Aug 25, 2020

Conversation

arturbosch
Copy link
Member

@arturbosch arturbosch commented Aug 9, 2020

This PR introduces a new module custom-checks with for now one rule SpekTestDiscovery.
This rule finds variable declarations in spek tests where memoized would help to improve the performance.

All previous PR's improved the discovery time by ~200ms per module (500-1000ms in extreme cases like metrics and rules-style modules (parsing Kotlin files)).
In conclusion nearly every object we use to setup tests has to be considered as "heavy".
Exceptions are template strings and paths. I've excluded them from the rule.

Test frameworks like Spek which not only use reflection to find the tests but also need to instantiate a class and run the constructor closure are errorprone to such issues if one forgets to use memoized or similar concepts.

@arturbosch arturbosch added this to the 1.11.0 milestone Aug 9, 2020
@codecov
Copy link

codecov bot commented Aug 9, 2020

Codecov Report

Merging #2954 into master will decrease coverage by 0.35%.
The diff coverage is 44.87%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2954      +/-   ##
============================================
- Coverage     79.78%   79.42%   -0.36%     
- Complexity     2506     2517      +11     
============================================
  Files           427      430       +3     
  Lines          7534     7612      +78     
  Branches       1420     1437      +17     
============================================
+ Hits           6011     6046      +35     
- Misses          767      794      +27     
- Partials        756      772      +16     
Impacted Files Coverage Δ Complexity Δ
...ithub/detekt/custom/CustomChecksConfigValidator.kt 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...in/io/github/detekt/custom/CustomChecksProvider.kt 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...otlin/io/github/detekt/custom/SpekTestDiscovery.kt 64.81% <64.81%> (ø) 11.00 <11.00> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a52518f...b9f8c7a. Read the comment docs.

@arturbosch arturbosch force-pushed the test-discovery-check branch 3 times, most recently from 7e89f23 to 66c9afe Compare August 9, 2020 18:26
@arturbosch arturbosch marked this pull request as ready for review August 9, 2020 19:31
@arturbosch arturbosch modified the milestones: 1.11.0-RC2, 1.11.0, 1.12.0 Aug 9, 2020
@arturbosch arturbosch force-pushed the test-discovery-check branch from 66c9afe to b9f8c7a Compare August 24, 2020 12:22
@arturbosch
Copy link
Member Author

Rule works and all our tests are free of expensive constructs.
The rule needs type resolution so it will only run on detektTest etc

I will setup a CI job with type resolution next. For this other rule issues need to be fixed.

@arturbosch arturbosch added housekeeping Marker for housekeeping tasks and refactorings and removed blocked labels Aug 24, 2020
@arturbosch arturbosch merged commit 609e411 into master Aug 25, 2020
@arturbosch arturbosch deleted the test-discovery-check branch August 25, 2020 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Marker for housekeeping tasks and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant