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

Fixed most reported issues by SonarCloud. #1492

Merged
merged 11 commits into from
Aug 21, 2020
Merged

Fixed most reported issues by SonarCloud. #1492

merged 11 commits into from
Aug 21, 2020

Conversation

ImLynxie
Copy link
Contributor

SonarCloud Issue fixes

  • This pull request is to resolve most of the issues that SonarCloud has picked up.

Pull request description

  • I've refactored most of the test cases that did not meet naming convention standards, attempted to fix the Cognitive Complexity issues within certain files, I haven't touched all of them as I was uncertain of the functionality. Mainly I've extracted parts of the original methods into their own methods to solve readability issues. I've also changed the unnecessary higher ranking access modifiers withing test cases, no changes were made to classes though, as I have no idea whether or not those methods should be used outside of the package/class itself.

@ImLynxie
Copy link
Contributor Author

I've fixed an issue that was causing build failures, which regarded the order of imports within a class, that's been resolved but now there seems to be a new error causing build failures, and I don't think it has something to do directly with any of the code I've modified.

I've searched for the file that it's asking for, and it is inside the project directory.

[INFO] --- maven-install-plugin:2.4:install (default-install) @ model-view-presenter ---
[INFO] Installing C:\Users\Toxic Dreamz\Documents\Development Projects\java-design-patterns\model-view-presenter\target\model-view-presenter.jar to C:\Users\Toxic Dreamz.m2\repository\com\iluwatar\model-view-presenter\1.23.0-SNAPSHOT\model-view-presenter-1.23.0-SNAPSHOT.jar
[INFO] Installing C:\Users\Toxic Dreamz\Documents\Development Projects\java-design-patterns\model-view-presenter\pom.xml to C:\Users\Toxic Dreamz.m2\repository\com\iluwatar\model-view-presenter\1.23.0-SNAPSHOT\model-view-presenter-1.23.0-SNAPSHOT.pom
[INFO]
[INFO] --- license-maven-plugin:3.0:format (install-format) @ model-view-presenter ---
[INFO] Updating license headers...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 15.332 s
[INFO] Finished at: 2020-08-15T22:15:38+04:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal com.mycila:license-maven-plugin:3.0:format (install-format) on project model-view-presenter: Resource license-plugin-header-style.xml not found in file system, classpath or URL: no protocol: license-plugin-header-style.xml -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException

Copy link
Owner

@iluwatar iluwatar left a comment

Choose a reason for hiding this comment

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

I think at the moment build is failing due to unit test failure in Extension Objects pattern. See the attached image.
image

@ImLynxie
Copy link
Contributor Author

Looks to me like it's a white space issue, there was a similiar whitespace issue within either the commander class or the sergeant class which caught my eye while reading to to write test cases for them, seems like I missed this one. I'll fix this right now, and push the changes.

@ImLynxie
Copy link
Contributor Author

Issue has been fixed and pushed, I've tested the test case locally, and it has passed.

@ImLynxie ImLynxie requested a review from iluwatar August 16, 2020 19:03
Copy link
Owner

@iluwatar iluwatar left a comment

Choose a reason for hiding this comment

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

The build is still failing. This time it seems to be a Checkstyle issue in Unit of Work pattern.

@ImLynxie
Copy link
Contributor Author

In that case, I'll just keep building until I don't run into any issues. I'll request a review after those build errors are gone.

@iluwatar
Copy link
Owner

You can detect Checkstyle issues faster using IDE. See instructions here: https://github.com/iluwatar/java-design-patterns/wiki/12.-IDE-instructions

@ImLynxie
Copy link
Contributor Author

I am using IntelliJ IDEA, and the checkstyle issues have been resolved fully, I have built the whole project locally without errors.

image

@ImLynxie ImLynxie requested a review from iluwatar August 19, 2020 09:29
@ImLynxie
Copy link
Contributor Author

There is an issue that is also causing build failures but I don't exactly know of a right way to approach this.

image

This issue is being caused by <version></version> tags not being present within the child modules pom.xml, I'm thinking it's causing the child modules jupiter dependency to default to another version instead of using the parent modules jupiter version, this isn't the case with all modules, some seem to be just throwing these errors and some don't.

How To Recreate

  • Build the project with maven

Known Ways to Resolve Issue

  • Add <version>5.5.2</version> to the jupiter dependency within the child modules pom.xml.

Example

<dependency>
          <groupId>org.junit.jupiter</groupId>
          <artifactId>junit-jupiter-engine</artifactId>
          <version>5.5.2</version>
          <scope>test</scope>
</dependency>

Copy link
Owner

@iluwatar iluwatar left a comment

Choose a reason for hiding this comment

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

Partial Response build fails because you are trying to mix JUnit 4 with JUnit 5. I think the current tests in that module have been written for JUnit 4. So either upgrade everything to 5 or stick with version 4.

You can detect the version from the imports. JUnit 4 imports look like this:

import static org.junit.Assert.assertEquals;
import org.junit.Before;
import org.junit.Test;

Whereas tests written for JUnit 5 have this kind of imports:

import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;

@ImLynxie
Copy link
Contributor Author

ImLynxie commented Aug 19, 2020

Unfortunately this was not the case, I've upgraded everything within that module to use JUnit 5, and it was still failing to build. The issue was being caused by an older version of the following dependency`

<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>

After changing that specific dependencies version to use version 5.4.0, the build was successful. Although I can't say if the issue was caused by mixing JUnit 4 and Junit 5, as I've only changed the version of the dependency after I upgraded everything to use JUnit 5

@ImLynxie ImLynxie requested a review from iluwatar August 19, 2020 21:31
dirty-flag/pom.xml Show resolved Hide resolved
partial-response/pom.xml Show resolved Hide resolved
pom.xml Show resolved Hide resolved
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-engine</artifactId>
<version>5.0.0</version>
Copy link
Owner

Choose a reason for hiding this comment

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

Use the one from parent pom.xml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you would like me to use the version from the parent pom, then the version within the parent pom would have to be updated, as that version is causing build issues within this module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other issues have been resolved and I will not push any changes just yet, as I'd like to know if you want to open a new issue regarding updating the classes that rely on the older version of junit-jupiter-engine.

Copy link
Owner

Choose a reason for hiding this comment

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

Acknowledge that. Then it's ok to leave as it is at this point. I'll create a follow-up issue to address the version problems.

@iluwatar iluwatar added this to the 1.23.0 milestone Aug 21, 2020
@iluwatar iluwatar merged commit 0c552ef into iluwatar:master Aug 21, 2020
@iluwatar
Copy link
Owner

This is part of task #1012. Created follow-up issue #1500.

@iluwatar
Copy link
Owner

@all-contributors please add @ToxicDreamz for code

@allcontributors
Copy link
Contributor

@iluwatar

I've put up a pull request to add @ToxicDreamz! 🎉

@iluwatar
Copy link
Owner

Many thanks @ToxicDreamz for looking into this issue. Let's see how the Sonar analysis looks after merging this!

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

Successfully merging this pull request may close these issues.

None yet

2 participants