[JENKINS-54424] - Remove Jenkins Test Harness Dependency & replace by local code #36
Conversation
… local code This change removes dependency on the Jenkins Test Harness framework. JUnit is still used, but there is a separate ticket for that. * Many classes were copied from JTH, but it can be optimized later * Irrelevant code in JenkinsRule and other classes was removed There is still a lot of optimizations/refactorings to be done (e.g. prevent WAR exploder from running at all, remove interfaces), but at least we keep the engine separate from JTH going forward
| return t; | ||
| } | ||
| }))); | ||
| QueuedThreadPool queuedThreadPool = new QueuedThreadPool(10); |
There was a problem hiding this comment.
Removed custom Thread Pool like @olamy did for Winstone in jenkinsci/winstone#54 . Likely we need to upstream the fix to Jenkins Test Harness so that it runs reliably on 50+ core machines. I wish we had them on our infra :P
FYI @jglick
| /** | ||
| * Allocates a new empty directory, meaning this will emulate the fresh Hudson installation. | ||
| */ | ||
| HudsonHomeLoader NEW = new HudsonHomeLoader() { |
| File src = new File(source.toURI()); | ||
| if(src.isDirectory()) | ||
| new FilePath(src).copyRecursiveTo("**/*",new FilePath(target)); | ||
| else |
There was a problem hiding this comment.
Might want to add some curly braces here for clarity.
| * @param <T> | ||
| * The recipe annotation associated with this runner. | ||
| */ | ||
| abstract class Runner<T extends Annotation> { |
There was a problem hiding this comment.
Not sure about compatibility, but would it be possible to refactor this into an interface with default methods?
There was a problem hiding this comment.
Would prefer to create a follow-up task for now
| @@ -0,0 +1,57 @@ | |||
| package io.jenkins.jenkinsfile.runner.util; | |||
| @@ -0,0 +1,16 @@ | |||
| package io.jenkins.jenkinsfile.runner.util; | |||
| * Allocates temporary directories and cleans it up at the end. | ||
| * @author Kohsuke Kawaguchi | ||
| */ | ||
| public class TemporaryDirectoryAllocator { |
There was a problem hiding this comment.
Might be too late at this point, but doesn't the JUnit TemporaryFolder rule do this, too?
There was a problem hiding this comment.
My plan is to get rid of JUnit... and this implementation as well. We really need a really simple mapper to a fixed directory, no magic required for Jenkinsfile Runner
There was a problem hiding this comment.
There is a ton of stuff here that does not look necessary at all. I do not understand why we would bother to remove the JTH binary dependency, only to copy & paste all this stuff which only exists for tests and has no relevance to the purpose.
Would it not be simpler to throw out this repo and start with @ndeloof’s, which seems to have a cleaner design?
setup/src/main/java/io/jenkins/jenkinsfile/runner/JenkinsRule.java
Outdated
Show resolved
Hide resolved
setup/src/main/java/io/jenkins/jenkinsfile/runner/JenkinsRule.java
Outdated
Show resolved
Hide resolved
setup/src/main/java/io/jenkins/jenkinsfile/runner/JenkinsRule.java
Outdated
Show resolved
Hide resolved
setup/src/main/java/io/jenkins/jenkinsfile/runner/JenkinsRule.java
Outdated
Show resolved
Hide resolved
The code will change late to improve performance. By moving the code here, we have no risk of breaking the JTH use-cases
Performance, again. This repo, even it has a non-production design (powered by JUnit, yey!), gave me much better startup time than the one from @ndeloof . Assuming I find enough time, I want to gradually integrate Nicolas' codebase and to move performance tweaks to the Jenkins core directly where possible |
This change removes dependency on the Jenkins Test Harness framework. JUnit is still used, but there is a separate ticket for that.
There is still a lot of optimizations/refactorings to be done (e.g. prevent WAR exploder from running at all, remove interfaces), but at least we keep the engine separate from JTH going forward
https://issues.jenkins-ci.org/browse/JENKINS-54424