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-31967] Specifying decimal numbers for JUnit health factor in Pipeline snippets results in invalid code #116

Merged

Conversation

Projects
None yet
2 participants

@artem-fedorov artem-fedorov changed the title Fix supporting java standard types https://issues.jenkins-ci.org/browse/JENKINS-31967 Mar 20, 2017

@artem-fedorov artem-fedorov changed the title https://issues.jenkins-ci.org/browse/JENKINS-31967 Fix supporting java standard types Mar 20, 2017

@artem-fedorov artem-fedorov changed the title Fix supporting java standard types [JENKINS-31967] Specifying decimal numbers for JUnit health factor in Pipeline snippets results in invalid code Mar 20, 2017

@jglick

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

The Snippetizer fix is only cosmetic. The actual fix needs to be done in DescribableModel in structs.

@jglick

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

Could probably pick up a snapshot dependency on jenkinsci/structs-plugin#16 to do a proper integration test here.

@artem-fedorov

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

Can tell how I should do it?

@jglick jglick self-requested a review May 22, 2017

if (clazz == Boolean.class || clazz == Integer.class || clazz == Long.class) {
if (clazz == Boolean.class || clazz == Integer.class || clazz == Long.class ||
clazz == Float.class || clazz == Double.class ||
clazz == Byte.class || clazz == Short.class) {

This comment has been minimized.

Copy link
@jglick

jglick May 22, 2017

Member

IIRC there is some utility in Commons Lang for this.

This comment has been minimized.

Copy link
@jglick

jglick May 25, 2017

Member

(forget it)

@jglick

This comment has been minimized.

Copy link
Member

commented May 22, 2017

Can tell how I should do it?

Just change the dependency version on structs to 1.7-20170522.183630-2, which I just deployed with your PR.

@@ -66,6 +66,7 @@
import org.kohsuke.stapler.QueryParameter;
import org.kohsuke.stapler.Stapler;
import org.kohsuke.stapler.StaplerRequest;
import org.springframework.util.ClassUtils;

This comment has been minimized.

Copy link
@jglick

jglick May 23, 2017

Member

Mm, rather not use springframework please. Better just revert this last change.

@jglick

This comment has been minimized.

Copy link
Member

commented May 23, 2017

Can tell how I should do it?

If you want to do this, you need to not just have a dependency on new structs but an actual test relying on it (probably in SnippetizerTest). I think we already have a suitable test-scoped dependency on junit.

artem-fedorov added some commits May 24, 2017

pom.xml Outdated
@@ -103,7 +103,7 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>structs</artifactId>
<version>1.6</version>
<version>1.7-20170522.183630-2</version>

This comment has been minimized.

Copy link
@jglick

jglick May 25, 2017

Member

Released; use 1.7.

@jglick
Copy link
Member

left a comment

I think you were missing my point here. The patch to structs already demonstrates theoretical coverage. I was suggesting an integration test to prove that the actual case most commonly reported really works. For example in master this test

@Issue("JENKINS-31967")
@Test public void junit() throws Exception {
    JUnitResultArchiver a = new JUnitResultArchiver("*.xml");
    st.assertRoundTrip(new CoreStep(a), "junit '*.xml'");
    a.setHealthScaleFactor(0.5);
    st.assertRoundTrip(new CoreStep(a), "junit healthScaleFactor: 0.5, testResults: '*.xml'");
}

fails:

org.junit.ComparisonFailure: expected:<... healthScaleFactor: [0.5], testResults: '*.xm...> but was:<... healthScaleFactor: [<object of type java.lang.Double>], testResults: '*.xm...>

and I would expect it to succeed, which would demonstrate both that DescribableModel correctly handled double values, and that Snippetizer correctly rendered them as Groovy source.

@jglick

jglick approved these changes May 30, 2017

@jglick jglick merged commit b48af16 into jenkinsci:master May 30, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
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.