ExpediaGroup / jenkins-spock

Unit-test Jenkins pipeline code with Spock
https://javadoc.io/doc/com.homeaway.devtools.jenkins/jenkins-spock
Apache License 2.0
186 stars 73 forks source link

Update fast-classpath-scanner (2.8.1) to classgraph (4.8.104) #99

Closed NingZhuRedsilence closed 3 years ago

NingZhuRedsilence commented 3 years ago

Summary

fast-classpath-scanner has been deprecated; classpath is its successor. Replacing fast-classpath-scanner with classgraph caused a few code changes because the classgraph API has changed.

Additional Details

Before changing to classpath, with fast-classpath-scanner up to version 3.1.13, unit tests written using JenkinsPipelineSpecification was throwing

java.lang.OutOfMemoryError: GC Overhead Limit Exceeded

at this line.

Checklist

Testing

(Remove this checklist and replace it with "N/A - no code changes" if this PR does not modify source code)

Documentation

(Remove this checklist and replace it with "N/A - no code changes" if this PR does not modify source code)

NingZhuRedsilence commented 3 years ago
  1. Do existing unit tests cover the correctness of the classpath-scanning code?
  2. Is it possible to add a unit test to duplicate the java.lang.OutOfMemoryError: GC Overhead Limit Exceeded exception against the original fast-classpath-scanner-based codebase, such that the test does not trigger the exception against the classgraph codebase?
  1. I don't think any of the existing *Spec.groovy tests cover the correctness of the classpath-scanning code; there seems to be no Java test in the repo for now.
  2. It would be possible to add a unit test as asked for, if we can recreate the state of the JVM where that error happened. But I don't know how at this moment.

However, since the newly "added" code is just calling a third-party library, I think we / I don't need to write unit test for this third-party code.

What in jenkins-spock are missing unit tests are method getClassesOfTypeInPackage and getClassesWithAnnotationOfTypeInPackage. I want to assert that adding unit tests to these 2 methods are a different and separate issue from this PR.

corporate-gadfly commented 3 years ago

According to ScanResult lifecycle, ScanResult should be assigned in a try-with-resources block. YMMV.