[Jenkins-39822] GitHub plugin functional tests broken against 1.651+ #157
Conversation
|
Plugin is on 1.609 and tests are fine. Security 170 could be fixed in other way. |
|
@KostyaSha I understand, but given that these changes do not upgrade the baseline nor they broke the existing tests I thought it could be interesting to add them. I do not understand the SECURITY-170 comment, are you referring that you prefer a separate PR for that or is that you prefer another solution? If you prefer another solution can you give me a hint, please? |
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| @@ -96,7 +97,7 @@ public void testNoBuildRevision() throws Exception { | |||
| FreeStyleProject prj = jRule.createFreeStyleProject(); | |||
| prj.setScm(new GitSCM("http://non.existent.git.repo.nowhere/repo.git")); | |||
| prj.getPublishersList().add(new GitHubCommitNotifier()); | |||
| Build b = prj.scheduleBuild2(0).get(); | |||
| Build b = prj.scheduleBuild2(0, new Cause.UserIdCause(), new BuildData()).get(); | |||
There was a problem hiding this comment.
On 2.x when the testNoBuidRevision executes there is no BuildData attached to the run, build.getAction(BuildData.class); returns null on ObjectId#getCommitSHA1 so the end result is exactly the same as when testNoBuildData is executed
So instead of trying to mock GitHub or change any version I thought the best curse of action is to make sure there is always a non null BuildData on that test
There was a problem hiding this comment.
So maybe create conditional on jenkins version?
| @@ -43,6 +46,9 @@ public void shouldExpandEnvAndBuildVars() throws Exception { | |||
| )); | |||
|
|
|||
| FreeStyleProject job = jRule.createFreeStyleProject(); | |||
| ParameterDefinition paramDef = new StringParameterDefinition(CUSTOM_BUILD_PARAM, "", ""); | |||
| ParametersDefinitionProperty paramsDef = new ParametersDefinitionProperty(paramDef); | |||
| job.addProperty(paramsDef); | |||
If github-plugin inject some parameters, then they should be fixed in other way instead of injecting in job bypassing properties. Good example is in https://github.com/jenkinsci/gerrit-trigger-plugin/pull/285/files#diff-54e50e5c447d3a893fd786bd419ee8b4R264 |
|
@KostyaSha Will work on your requested changes. Thanks for your time |
Its not. Just expands some vars for status setter |
|
@KostyaSha It seems that the null build data problem does not come from jenkins core but git plugin. To be able to execute against core 2.19.3 you have to use a newer version of git. Git plugin since 2.4.1 does not store BuildData if checkout goes wrong (see this commit). That means a conditional does not make sense, unless is removed when git dependency is upgraded, which IMHO results in an unneeded TODO and ugly code So, this PR does not make the existing test broke and if for some reason git dependency is upgraded it makes the test work against it without code changes, but is unnecessary for the current version of the dependency. Do not know if you plan to upgrade git dependency shortly (to 2.4.4 for example) or not. If you plan that it may made sense to continue with this PR, if not it may not and I will change it to only fix the SECURITY-170 issue. Opinions? |
|
I think we can try to update to git plugin of latest 2.x.x |
|
Just add conditional on plugin version. And wrap this place. Examples should be somewhere in core/plugins. |
* This way we change behaviour only if needed
* Git plugin 2.4.1+ does not include BuildData if checkout fails, resulting in testNoBuildRevision failing
|
@KostyaSha @lanwen Tried to address your comments. Is the code better now?? |
|
Wow, as for me previous version was much better, @KostyaSha |
After few years you will say thanks :D but still there is no meaningful description WHY this hacks are needed. Security170 is not a description, what exactly and why? |
| import java.lang.reflect.Field; | ||
| import java.lang.reflect.Modifier; | ||
|
|
||
| public class ParametersActionHelper { |
There was a problem hiding this comment.
please add javadoc with complete description for such classes
|
@KostyaSha @lanwen Clarifying comments and javadoc added. |
|
Please also make codacy happy :) |
|
@lanwen maybe comments should be in Java doc for methods? |
|
No, i think in this case they should be inlined (now is ok) |
JENKINS-39822
@reviewbybees
This change is