Skip to content
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

[JENKINS-22252] Reproduced problem in test #41

Merged
merged 1 commit into from Oct 2, 2015

Conversation

jglick
Copy link
Member

@jglick jglick commented Sep 30, 2015

JENKINS-22252

@reviewbybees

@ghost
Copy link

ghost commented Sep 30, 2015

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.

@@ -246,4 +248,20 @@ private void assertHasModule(MavenModuleSet job, String name) {
find(by.xpath("//a[@href='%s/']", name)).click();
assertThat(driver.getCurrentUrl(), equalTo(build.module(name).url.toExternalForm()));
}

@Issue("JENKINS-22252")
@WithPlugins({"maven-plugin@2.12", "tasks"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the plugin version specified for some reason? If the plugin version is fixed here, then this test is not going to catch possible regressions in future versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. This is actually reproducing the error exactly in this version. Nevermind.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, reading the code again... I think my initial question still makes sense 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this version specification is a temporary thing and @jglick is going to update it to 2.13 after the release. Before that this test will be failing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be the initial version required to get the test running? I.e., 2.12-SNAPSHOT or 2.13? It is not required to change it when 2.13 has been released. So we can better see in which version the fix has been applied.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be the first version where necessary features was introduced. As it is a regression introduced in 2.12 IIUC, there should be no version specified (presuming it does not depend on any recent features).

As a rule of thumb, consider someone is running this against a plugin version with JENKINS-22252 present. If we declare this should run with first fixed version we effectively hide the presence of the bug from the user instead of pointing his attention to it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be no version specified (presuming it does not depend on any recent features)

It does not depend on any recent features. But if you do not specify a version, then the test will not test the version on the update center, currently 2.12. Instead it will use the 2.7.1 or whatever bundled¹ with jenkins.war. This was the way I found to force it to install version 2.12 or newer so that the test is meaningful.

¹One of the many irritating consequences of plugin bundling—hope to see it die soon.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense then, we should remove once we get rid of bundled plugins.

@stephenc
Copy link
Member

stephenc commented Oct 2, 2015

🐝

@jglick
Copy link
Member Author

jglick commented Oct 2, 2015

Now passes with 2.12.1 on the update center.

jglick added a commit that referenced this pull request Oct 2, 2015
@jglick jglick merged commit aac7425 into jenkinsci:master Oct 2, 2015
@jglick jglick deleted the IllegalAccessError-JENKINS-22252 branch October 2, 2015 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants