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.

Sign up for GitHub

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

Jump to bottom

Implement AxivionSuite tool which queries the Axivion Dashboard for violations #20

Merged

uhafner merged 13 commits into jenkinsci:master from arturbosch:support-axivion-parser

Mar 29, 2019

Contributor

arturbosch commented Mar 23, 2019

No description provided.


          Implement AxivionSuite tool which queries the Axivion Dashboard for v…

7d8dad3

…iolations

codecov bot commented Mar 23, 2019 •

edited

Loading

Codecov Report

Merging #20 into master will decrease coverage by 1.42%.
The diff coverage is 55.67%.

@@             Coverage Diff             @@
##             master     #20      +/-   ##
===========================================
- Coverage     80.83%   79.4%   -1.43%     
- Complexity     1371    1393      +22     
===========================================
  Files           225     231       +6     
  Lines          4685    4967     +282     
  Branches        375     394      +19     
===========================================
+ Hits           3787    3944     +157     
- Misses          744     864     +120     
- Partials        154     159       +5

Impacted Files	Coverage Δ	Complexity Δ
...lysis/warnings/axivion/RemoteAxivionDashboard.java	`0% <0%> (ø)`	`0 <0> (?)`
.../plugins/analysis/warnings/axivion/AxRawIssue.java	`100% <100%> (ø)`	`6 <6> (?)`
...plugins/analysis/warnings/axivion/AxIssueKind.java	`100% <100%> (ø)`	`4 <4> (?)`
...lugins/analysis/warnings/axivion/AxivionSuite.java	`3.48% <3.48%> (ø)`	`0 <0> (?)`
...lysis/warnings/axivion/DefaultTransformations.java	`89.31% <89.31%> (ø)`	`8 <8> (?)`
...ugins/analysis/warnings/axivion/AxivionParser.java	`92.85% <92.85%> (ø)`	`4 <4> (?)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 371599f...f7d1d21. Read the comment docs.

uhafner requested changes

View reviewed changes

src/main/java/io/jenkins/plugins/analysis/warnings/axivion/AxivionParser.java Outdated

+              import net.sf.json.JSONArray;
+              import net.sf.json.JSONObject;
+              public class AxivionParser extends IssueParser implements Serializable {

Member

uhafner Mar 23, 2019

Why do you implement IssueParser?

src/main/java/io/jenkins/plugins/analysis/warnings/axivion/AxivionSuite.java Outdated

+                  private String credentialsId = StringUtils.EMPTY;
+                  private String basedir = "$";
+                  public AxivionSuite(final String projectUrl, final String credentialsId, final String basedir) {

Member

uhafner Mar 23, 2019

package private and @VisibleForTesting

src/main/java/io/jenkins/plugins/analysis/warnings/axivion/AxivionSuite.java Outdated

+                  public AxivionSuite(final String projectUrl, final String credentialsId, final String basedir) {
+                      super();
+                      // empty constructor required for stapler

Member

uhafner Mar 23, 2019

remove comment

src/main/java/io/jenkins/plugins/analysis/warnings/axivion/AxivionSuite.java Outdated

+                      return new AxivionParser(projectUrl,
+                              withValidateCredentials(),
+                              expandBaseDir(run, this.basedir)
+                      ).parse(null);

Member

uhafner Mar 23, 2019

null not required if parser does not implement interface.

src/main/java/io/jenkins/plugins/analysis/warnings/axivion/AxivionSuite.java

+              import org.apache.commons.lang3.StringUtils;
+              import org.apache.http.auth.UsernamePasswordCredentials;
+              import com.cloudbees.plugins.credentials.CredentialsMatchers;

Member

uhafner Mar 23, 2019

Where is this dependency coming from? Jenkins-core?

Contributor Author

arturbosch Mar 25, 2019

It comes from the Credentials Plugin which is shipped by Jenkins and can't be deinstalled.

src/main/java/io/jenkins/plugins/analysis/warnings/axivion/AxivionSuite.java Outdated

+              public class AxivionSuite extends Tool {
+                  private static final long serialVersionUID = 8670135727302169818L;
+                  static final String ID = "AxivionStoppingSoftwareErosion";

Member

uhafner Mar 23, 2019

quite a long URL, is this intended?

Contributor Author

arturbosch Mar 25, 2019

Yep, it was intented, but AxivionSuite is ok too. Changed it.

Member

uhafner Mar 25, 2019

It is used as URL to the results, so typically it does contain only lowercase characters (separated by -), see https://github.com/jenkinsci/warnings-ng-plugin/blob/master/SUPPORTED-FORMATS.md.

src/test/java/io/jenkins/plugins/analysis/warnings/axivion/AxivionParserTest.java Outdated

+                          assertTrue(ArrayUtils.contains(expected, actual.getType()));
+                      }
+                  }
+              }

Member

uhafner Mar 23, 2019

Some more tests would be helpful.

src/test/java/io/jenkins/plugins/analysis/warnings/axivion/AxivionParserTest.java Outdated

+                      };
+                      for (final Issue actual : report) {
+                          assertTrue(ArrayUtils.contains(expected, actual.getType()));
+                      }

Member

uhafner Mar 23, 2019

check all properties of an issue

src/test/java/io/jenkins/plugins/analysis/warnings/axivion/AxivionParserTest.java Outdated

+                      final String payload = new String(new Resource(svJson).asByteArray());
+                      Report report = new Report();
+                      parser.processIssues(

Member

uhafner Mar 23, 2019

why don't you test the parse() method?

arturbosch added 7 commits

March 25, 2019 10:25


          Do not extend IssueParser simplifying parser setup

e042eb1


          Address review feedback


          Pull up dashboard object from parser

c5718a0


          Make AxivionParser more generic by pulling up extracting the issue ki…

2335ef7

…nd traversal logic


          Extract issue transformations from transformation interface

c631451


          Test all axivion issue kinds

ce068cb


          Change to package private modifiers

77bb647

Contributor Author

arturbosch commented Mar 25, 2019

Thanks for the review, I incorporated your comments.

What is the right way to integrate jelly templating?
When this plugin was a standalone plugin jelly worked, I had a hard time migrating the jelly files to the new resources package structure. Lastly it worked, now after renaming the ID field, my custom three input fields again disappeared.
The parser still works for already configured projects.

I can't reproduce the architecture violations locally.
On CI it tells me not to use Jenkins.getInstance(). What is the prefered way to retrieve the credentials ids from jenkins?

Is assertAll not allowed in your code base due to longer CI runs?


          Document test code

91d1c45

Member

uhafner commented Mar 25, 2019 •

edited

Loading

What is the right way to integrate jelly templating?
When this plugin was a standalone plugin jelly worked, I had a hard time migrating the jelly files to the new resources package structure. Lastly it worked, now after renaming the ID field, my custom three input fields again disappeared.
The parser still works for already configured projects.

See https://github.com/jenkinsci/warnings-ng-plugin/pull/20/files#r268879821

I can't reproduce the architecture violations locally.
On CI it tells me not to use Jenkins.getInstance(). What is the prefered way to retrieve the credentials ids from jenkins?

The architecture violations are from a simple unit test, why does it not work locally?

I'm currently preparing the project to be used in my testing course here at the university, so I want that everybody conforms to my guidelines. I hope they are not too academic;-) Two new rules that are now enforced with an architecture test:

I generally want to forbid direct Jenkins.getInstance from code since these code parts cannot be unit tested (you need an integration test). In order to make auch a code testable you need to call the Jenkins façade that handles these calls. The Jenkins façade can be replaced in a unit test with a stub. If your method to Jenkins instance is not available yet, please add it to the façade. Here is an example: https://github.com/jenkinsci/warnings-ng-plugin/blob/master/src/test/java/io/jenkins/plugins/analysis/core/util/ModelValidationTest.java

Is assertAll not allowed in your code base due to longer CI runs?

No, I simply want to use only one assertion framework in my modules and that is AssertJ. Otherwise contributors get confused if some use JUnit4, some use JUnit5, some AssertJ, etc.

uhafner reviewed

View reviewed changes

src/main/resources/io/jenkins/plugins/analysis/warnings/axivion/local-config.jelly Outdated Show resolved Hide resolved

arturbosch added 3 commits

March 26, 2019 19:14


          Fix broken jelly properties

6aaac96


          Migrate test assertions to AssertJ

1924e59


          Replace Jenkins.getInstance with JenkinsFacade to be compliant with a…

ddab005

…rchitecture check

Contributor Author

arturbosch commented Mar 26, 2019 •

edited

Loading

Thanks for the input, resources/AxivionSuite/config.jelly and ID=axivion-suite made it work.

Just the "document all public members" I would call too academic as I remember this requirement from my studies in Bremen ;).
All other checks are impressive, need to look into the architecture checks for detekt too :).

Member

uhafner commented Mar 28, 2019

Seems that your tests (or your code;-) do not work correctly on Windows:

https://ci.jenkins.io/job/Plugins/job/warnings-ng-plugin/job/PR-20/7/testReport/io.jenkins.plugins.analysis.warnings.axivion/AxivionParserTest/


          Use internal setDirectory and setFilename for path normalization

f7d1d21

arturbosch force-pushed the support-axivion-parser branch from 587019e to f7d1d21 Compare

March 28, 2019 10:32

uhafner approved these changes

View reviewed changes

uhafner merged commit c755bc0 into jenkinsci:master

Member

uhafner commented Mar 29, 2019

Thanks!

arturbosch deleted the support-axivion-parser branch

April 1, 2019 05:50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet