Fix local class scanning#111
Open
wheelerlaw wants to merge 2 commits into
Open
Conversation
|
I am not sure if this related, but after adding some dependencies to one of my projects, an old test using jenkins-spock is no longer working, complaining about missing OSGi and JNA classes while using the org.reflections tool. I have no idea why this is happening. Would this PR fix a problem like this? |
Answering my own question: Yes, this PR fixes my problem. I just cloned the fork and built this PR's branch. I think it would make a lot of sense for the maintainer to actually review and, if it proves to be valid, merge this PR. |
awittha
reviewed
Nov 29, 2021
Contributor
awittha
left a comment
awittha
left a comment
There was a problem hiding this comment.
Looks good, but I have some questions I need cleared up before I can either approve or ask for changes.
Comment on lines
-274
to
-278
| <dependency> | ||
| <groupId>org.reflections</groupId> | ||
| <artifactId>reflections</artifactId> | ||
| <version>0.9.12</version> | ||
| </dependency> |
Contributor
There was a problem hiding this comment.
The motivation behind using Reflections AND GlassGraph, was that Reflections would only see files in the local project; nothing on the extended classpath from Maven dependencies would be included.
How does one configure the current ClassGraph to only see files that are physically present on disk in the current project?
Or, to ask another way:
With this code change, if there's a dependency of the project that includes a file like the ScriptToTest.groovy that's added here, will these changes detect that script, or not?
They should not.
Is that done with the .disableJarScanning() here: https://github.com/ExpediaGroup/jenkins-spock/pull/111/files#diff-7cf77f094673fdcead882d1c0b654a186428288727ba01a59de640f2a9f80d7fR57 ?
| } | ||
| Set<Class<?>> annotated_classes; | ||
| try ( | ||
| ScanResult scanResult = new ClassGraph() |
Contributor
There was a problem hiding this comment.
Should this also .disableJarScanning()?
|
|
||
| class LocalProjectPipelineExtensionDetectorSpec extends JenkinsPipelineSpecification { | ||
|
|
||
| def "LocalProjectPipelineExtensionDetectorSpec should detect classes that aren't direct subtypes of java-lang-Object" () { |
Contributor
There was a problem hiding this comment.
Suggested change
| def "LocalProjectPipelineExtensionDetectorSpec should detect classes that aren't direct subtypes of java-lang-Object" () { | |
| def "LocalProjectPipelineExtensionDetector should detect classes that aren't direct subtypes of java-lang-Object" () { |
It's really the LocalProjectPipelineExtensionDetector that you're testing, right?
If you were testing the LoalProjectPipelineExtensionDetectorSpec, you'd be checking the DEFAULT_TEST_CLASSES ( https://github.com/ExpediaGroup/jenkins-spock/blob/master/src/main/groovy/com/homeaway/devtools/jenkins/testing/JenkinsPipelineSpecification.groovy#L1137 ) variable's contents, right?
I think this test can actually just extend Specification, since it's testing a standalone, non-Jenkins-specific class (LocalProjectPipelineExtensionDetector), right?
And if that's the case... since the class-under-test is in Java, can the test itself be written in ("matching") JUnit Java, too?
(These are actual questions, not change requests phrased as questions, by the way.)
|
Why is nobody fixing this problem after such a long time? Locally, I am working with a snapshot version containing this fix, but my CI builds, e.g. on GitHub, cannot use that snapshot, because it is not deployed in any publicly available repository. So I have a choice to either skip my Jenkins Spock tests or let them fail with the error I posted above. It would be nice to get a maintenance release after almost 4 months. With regard to code review feedback: Why not just be pragmatic, merge locally and fix what you think ought to be fixed, like a normal maintainer would, taking repsonsibility for what is merged into his own repository? You do not need to wait for the PR creator to commit your suggestions. |
kriegaex
added a commit
to kriegaex/GebSpockSamples
that referenced
this pull request
Feb 17, 2022
Use snapshot locally, but release on GitHub in order to avoid that Maven cannot find the snapshot during the build. At the same time, PublishBuildLogIT will be skipped if "jenkins-spock-2.1.5.jar" is found on the classpath. I.e., it will be skipped on GitHub, because with that buggy dependency it will fail. We can remove that restriction after ExpediaGroup/jenkins-spock#111 was merged or the corresponding bug fixed otherwise and a new release published.
kriegaex
added a commit
to kriegaex/GebSpockSamples
that referenced
this pull request
Feb 18, 2022
Use snapshot locally, but release on GitHub in order to avoid that Maven cannot find the snapshot during the build. At the same time, PublishBuildLogIT will be skipped if "jenkins-spock-2.1.5.jar" is found on the classpath. I.e., it will be skipped on GitHub, because with that buggy dependency it will fail. We can remove that restriction after ExpediaGroup/jenkins-spock#111 was merged or the corresponding bug fixed otherwise and a new release published.
priyankarawat
added a commit
to konciergeMD/jenkins-spock
that referenced
this pull request
Jun 27, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: The branch for this PR is rebased on top of the branch for the #110 because of the importance of fixing the master build process.
Summary
There are a few changes in this PR:
Jenkins allows shared libraries to be written as a script, in addition to fully qualified class. The superclass of a script is
groovy.lang.Script, whereas the superclass of a regular class is whatever the class extends (orjava.lang.Objectif it doesn't extend a class).The
LocalProjectPipelineExtensionDetectorclass scans for classes in the project to instrument in thegetClassesOfTypeInPackagemethod by using the Reflections package. It scans for classes that extend Object. Reflections, however, does not supporting scanning for classes that extend a class but are not direct subtypes of the same class. So thegetClassesOfTypeInPackagemethod misses all Jenkins scripts that are a part of the shared library.An easy solution to this would be to also scan for classes that extend
groovy.lang.Script. However, it is also perfectly valid for Jenkins shared libraries classes to extend other Jenkins shared library classes. The solution here is to use a class scanner that supports searching for classes that are descendants of a class.According to this comment, a
ScanResultshould be assigned within a try-with-resources, so I have made that change in the Local detector and the WholeClasspath detector.I found another Java 11+ compatibility issue (not catching
NoClassDefFoundErrorexception) and fixed it.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)
CHANGELOG.mdwith a brief description of my changes.CONTRIBUTING.mdand have followed its guidance.