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

Adapt to new plugin parent pom #281

Merged
merged 8 commits into from May 17, 2016

Conversation

Projects
None yet
3 participants
@rsandell
Copy link
Member

commented Apr 28, 2016

This makes it easier to test on newer core versions.

Besides the pom mostly javadoc and jelly fixes but usage of HtmlUnit needed to be tweaked as well.

When playing around to find a fix for the failing configRoundTrip test on workflow I needed to change the access of Queue.Item.id that indicated a potential hidden bug, so I decided to bump dependency to 1.609.3 and fix it.

@reviewbybees
@TWestling

rsandell added some commits Apr 27, 2016

Fixed failing unit tests
Decided to keep dependency to 1.609.3 because of
changes in queue.item.id that indicates a hidden potential
bug in the plugin.
@reviewbybees

This comment has been minimized.

Copy link

commented Apr 28, 2016

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.

@rsandell

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

Hmm, it is a bit disturbing to me that the tests now timeout and runs much slower. They pass on my machine, but runs slower, and only on jdk8. On jdk7 they run out of perm gen.
Seems like the ci builds are having a hard time even when running on an m1.large.

@rsandell

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

Hmm again, the timeouts could be due to com.jcraft.jsch.JSchException: Algorithm negotiation fail and java.lang.IllegalStateException: Unable to negotiate key exchange for encryption algorithms, but this is java running an ssh command to the same java process. So I guess there are no available algorithms at all? When did this happen?

@rsandell

This comment has been minimized.

Copy link
Member Author

commented May 9, 2016

Ok, so as it stands now, running with jdk8 fails due to the above algorithm exchange error. And jdk 7 fails due to not enough memory. Will see if I can turn off reuse forks et.al. to get he memory consumption down.

<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>junit</artifactId>
<version>1.12</version>
<scope>test</scope>

This comment has been minimized.

Copy link
@jglick

jglick May 9, 2016

Member

Why? All plugins get this dep anyway.

This comment has been minimized.

Copy link
@rsandell

rsandell May 9, 2016

Author Member

Doesn't seem so, the plugin wasn't loaded when the unit tests ran, so I had to add it explicitly.

This comment has been minimized.

Copy link
@jglick

jglick May 9, 2016

Member

Ah, org.jenkins-ci.plugins:junit not junit:junit. Ignore me.

<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-step-api</artifactId>
<version>1.4</version>
<version>${workflow.version}</version>

This comment has been minimized.

Copy link
@jglick

jglick May 9, 2016

Member

A little dangerous, but OK for now.

@@ -298,25 +290,6 @@
</executions>

This comment has been minimized.

Copy link
@jglick

jglick May 9, 2016

Member

build-helper-maven-plugin configuration looks like it might be unnecessary now, or could be moved to parent POM.

@@ -1,3 +1,4 @@
<?jelly escape-by-default='true'?>

This comment has been minimized.

Copy link
@jglick

jglick May 9, 2016

Member

Remember that this change can cause regressions. If the original file had

${it.someUntaintedDataWhichMightContainHTML}

you need to switch it to

<j:out value="${it.someUntaintedDataWhichMightContainHTML}"/>

This comment has been minimized.

Copy link
@rsandell

rsandell May 9, 2016

Author Member

Yes, I took a quick scan through the files that I was changing, the only worry I have is how the data binding works for form values, e.g. if a textfield value contains "<" or ">"?

This comment has been minimized.

Copy link
@jglick

jglick May 9, 2016

Member

That is fine. You only need to look for cases where the view intends to display HTML content bound to some model object attribute (something.detailMessage, etc.), where the content is known be untainted by user input. Adding escape-by-default='true' without switching that usage to j:out will cause the content to be rendered as plain text rather than markup, breaking the UI.

authorizationStrategy.add(Hudson.READ, "authenticated");
j.getInstance().setAuthorizationStrategy(authorizationStrategy);
j.getInstance().setAuthorizationStrategy(
new MockAuthorizationStrategy().grant(Hudson.READ).everywhere().toAuthenticated());

This comment has been minimized.

Copy link
@jglick

jglick May 9, 2016

Member

Glad to see this get used!

This comment has been minimized.

Copy link
@rsandell

rsandell May 9, 2016

Author Member

I remembered it from a code review some time back, so I knew it was available :)

@@ -152,7 +152,7 @@ public void testReconfigureUsingRestApi() throws Exception {
Document document = xmlPage.getXmlDocument();
String xml = changeConfigXml(gerritProjectPattern, document);
URL url = UrlUtils.toUrlUnsafe(j.getURL().toExternalForm() + testProj.getUrl() + "config.xml");
WebRequestSettings request = new WebRequestSettings(url, HttpMethod.POST);
WebRequest request = new WebRequest(url, HttpMethod.POST);
request.setRequestBody(xml);
j.jenkins.setCrumbIssuer(null);

This comment has been minimized.

Copy link
@jglick

jglick May 9, 2016

Member

🐜 Better to use getCrumbedUrl.

@jglick

This comment has been minimized.

Copy link
Member

commented May 9, 2016

🐝

More test tweaking
Some tests had lost their @test annotation when being migrated
to JenkinsRule. Enabled and fixed them.
Fixed SshdServerMock to be able to run in Parallel and fixed
tests to adapt to it.
Migrated GerritTriggerTimer to use jenkins.util.Timer instead of
java.util.Timer because it was breaking some tests.
Tweaked sure fire to not reuse forks and the max heap for each suite.
@rsandell

This comment has been minimized.

Copy link
Member Author

commented May 13, 2016

So the tests are working again locally for me, and hopefully surefire is tweaked enough so that it can run on an old m1.large, waiting on Jenkins to see if it is so.

rsandell added some commits May 13, 2016

Specifying the key file in the tests
Might help them not timeout
Setting the ssh key property only works for already configured servers
Not if we create new server configurations
Also fixed some NPEs in GerritServer that should only appear in testing
@rsandell

This comment has been minimized.

Copy link
Member Author

commented May 16, 2016

Yay! All the tests are passing on CI 💃

@rsandell

This comment has been minimized.

Copy link
Member Author

commented May 16, 2016

@TWestling do you want to take a look first or can I merge without mercy?

@rsandell

This comment has been minimized.

Copy link
Member Author

commented May 17, 2016

Ping @TWestling again for testing's sake

@rsandell rsandell merged commit 9e96c77 into master May 17, 2016

1 check passed

Jenkins This pull request looks good
Details

@rsandell rsandell deleted the new-parent-pom branch May 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.