-
Notifications
You must be signed in to change notification settings - Fork 25
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
No java util date rule test #5
Changes from 2 commits
fd147a0
2804bb9
179c700
1051de7
3cf346d
644d9bd
ae1ba34
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,13 @@ | |
<name>Vincent Fuchs</name> | ||
<email>[email protected]</email> | ||
</developer> | ||
|
||
<developer> | ||
<id>FanJups</id> | ||
<name>Fanon Jupkwo</name> | ||
<email>[email protected]</email> | ||
</developer> | ||
|
||
</developers> | ||
|
||
<licenses> | ||
|
@@ -256,6 +263,8 @@ | |
<artifactId>coveralls-maven-plugin</artifactId> | ||
<version>4.3.0</version> | ||
</plugin> | ||
|
||
|
||
</plugins> | ||
</build> | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
package com.societegenerale.commons.plugin.rules; | ||
|
||
import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes; | ||
import static java.util.stream.Collectors.toList; | ||
|
||
import java.util.List; | ||
|
||
import com.societegenerale.commons.plugin.utils.ArchUtils; | ||
import com.tngtech.archunit.core.domain.JavaClass; | ||
import com.tngtech.archunit.core.domain.JavaField; | ||
import com.tngtech.archunit.core.domain.JavaMethod; | ||
import com.tngtech.archunit.core.domain.JavaMethodCall; | ||
import com.tngtech.archunit.lang.ArchCondition; | ||
import com.tngtech.archunit.lang.ConditionEvents; | ||
import com.tngtech.archunit.lang.SimpleConditionEvent; | ||
|
||
/** | ||
* java.util.Date is deprecated but a lot of people still use it out of years of | ||
* habit. This rule will catch such instances and remind developers they should | ||
* use alternatives (java.time, java.util.GregorianCalendar , | ||
* java.text.DateFormat (and its subclasses) to parse and format dates) because | ||
* they support internationalization better | ||
* | ||
* | ||
* | ||
* @see <a href= | ||
* "https://www.math.uni-hamburg.de/doc/java/tutorial/post1.0/converting/deprecated.html">java.util.Date | ||
* is deprecated</a> : <i>developers can use other libraries : java.time, | ||
* java.util.GregorianCalendar ; java.text.DateFormat ; ... </i> | ||
* | ||
* @see <a href= | ||
* "https://docs.oracle.com/javase/8/docs/api/java/time/package-summary.html">Java | ||
* 8 Time Oracle</a> | ||
*/ | ||
|
||
public class NoJavaUtilDateRuleTest implements ArchRuleTest { | ||
|
||
private static final String JAVA_UTIL_DATE_PACKAGE_PREFIX = "java.util.Date"; | ||
|
||
@Override | ||
public void execute(String path) { | ||
classes().should(notUseJavaUtilDate()) | ||
.check(ArchUtils.importAllClassesInPackage(path, ArchUtils.SRC_CLASSES_FOLDER)); | ||
} | ||
|
||
protected static ArchCondition<JavaClass> notUseJavaUtilDate() { | ||
|
||
return new ArchCondition<JavaClass>("not use Java Util Date ") { | ||
@Override | ||
public void check(JavaClass item, ConditionEvents events) { | ||
|
||
List<JavaField> classesWithJavaUtilDateFields = item.getAllFields().stream() | ||
.filter(field -> isJavaUtilDateField(field)).collect(toList()); | ||
|
||
for (JavaField field : classesWithJavaUtilDateFields) { | ||
events.add(SimpleConditionEvent.violated(field, ArchUtils.NO_JAVA_UTIL_DATE_VIOLATION_MESSAGE | ||
+ " - class: " + field.getOwner().getName() + " - field name: " + field.getName())); | ||
} | ||
|
||
List<JavaMethodCall> methodsUsingJavaUtilDateInternally = item.getCodeUnits().stream() | ||
.filter(codeUnit -> codeUnit instanceof JavaMethod) | ||
.flatMap(method -> method.getMethodCallsFromSelf().stream()) | ||
.filter(method -> method instanceof JavaMethodCall) | ||
.filter(method -> isMethodUsingJavaUtilDateInternally(method)).collect(toList()); | ||
|
||
for (JavaMethodCall methodCall : methodsUsingJavaUtilDateInternally) { | ||
events.add(SimpleConditionEvent.violated(methodCall.getOriginOwner(), | ||
ArchUtils.NO_JAVA_UTIL_DATE_VIOLATION_MESSAGE + " - class: " | ||
+ methodCall.getOriginOwner().getName() + " - line: " | ||
+ methodCall.getLineNumber())); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. code from lines 60-71 is not unit tested properly : I've commented it locally, and the tests are still passing. so please add a test for that use case, or remove that part of the code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a pleasure to contribute to this project. The source code format is automatically defined by Eclipse. I will also focus on these tests. Thanks for your idea. |
||
} | ||
|
||
@SuppressWarnings("deprecation") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove the SuppressWarnings annotation and modify your code so that no deprecation warning comes up. |
||
private boolean isJavaUtilDateField(JavaField field) { | ||
return field.getType().getName().startsWith(JAVA_UTIL_DATE_PACKAGE_PREFIX); | ||
} | ||
|
||
private boolean isMethodUsingJavaUtilDateInternally(JavaMethodCall javaMethodCall) { | ||
return javaMethodCall.getTarget().getFullName().startsWith(JAVA_UTIL_DATE_PACKAGE_PREFIX); | ||
} | ||
|
||
}; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,52 +1,53 @@ | ||
package com.societegenerale.commons.plugin.utils; | ||
|
||
import com.tngtech.archunit.base.DescribedPredicate; | ||
import java.nio.file.Path; | ||
import java.nio.file.Paths; | ||
|
||
import com.tngtech.archunit.core.domain.JavaClass; | ||
import com.tngtech.archunit.core.domain.JavaClasses; | ||
import com.tngtech.archunit.core.importer.ClassFileImporter; | ||
|
||
import java.nio.file.Path; | ||
import java.nio.file.Paths; | ||
|
||
/** | ||
* Created by agarg020917 on 11/17/2017. | ||
*/ | ||
public class ArchUtils { | ||
|
||
private static final String JUNIT4_ASSERT_PACKAGE_NAME = "org.junit.Assert"; | ||
private static final String JUNIT5_ASSERT_PACKAGE_NAME = "org.junit.jupiter.api.Assertions"; | ||
|
||
public static final String NO_JUNIT_ASSERT_DESCRIPTION = "not use Junit assertions"; | ||
private static final String JUNIT4_ASSERT_PACKAGE_NAME = "org.junit.Assert"; | ||
private static final String JUNIT5_ASSERT_PACKAGE_NAME = "org.junit.jupiter.api.Assertions"; | ||
|
||
public static final String TEST_CLASSES_FOLDER = "/test-classes"; | ||
public static final String SRC_CLASSES_FOLDER = "/classes"; | ||
private static final String PACKAGE_SEPARATOR = "."; | ||
public static final String NO_JUNIT_ASSERT_DESCRIPTION = "not use Junit assertions"; | ||
|
||
public static final String NO_PREFIX_INTERFACE_VIOLATION_MESSAGE = " : Interfaces shouldn't be prefixed with \"I\" - caller doesn't need to know it's an interface + this is a .Net convention"; | ||
public static final String POWER_MOCK_VIOLATION_MESSAGE= "Favor Mockito and proper dependency injection - "; | ||
public static final String NO_INJECTED_FIELD_MESSAGE = "Favor constructor injection and avoid field injection - "; | ||
public static final String NO_AUTOWIRED_FIELD_MESSAGE = "Favor constructor injection and avoid autowiring fields - "; | ||
public static final String NO_JODA_VIOLATION_MESSAGE = "Use Java8 Date API instead of Joda library"; | ||
public static final String NO_JUNIT_IGNORE_VIOLATION_MESSAGE = "Tests shouldn't been ignored"; | ||
public static final String NO_JUNIT_IGNORE_WITHOUT_COMMENT_VIOLATION_MESSAGE = "Tests shouldn't been ignored without providing a comment explaining why"; | ||
public static final String TEST_CLASSES_FOLDER = "/test-classes"; | ||
public static final String SRC_CLASSES_FOLDER = "/classes"; | ||
private static final String PACKAGE_SEPARATOR = "."; | ||
|
||
public static final String NO_PREFIX_INTERFACE_VIOLATION_MESSAGE = " : Interfaces shouldn't be prefixed with \"I\" - caller doesn't need to know it's an interface + this is a .Net convention"; | ||
public static final String POWER_MOCK_VIOLATION_MESSAGE = "Favor Mockito and proper dependency injection - "; | ||
public static final String NO_INJECTED_FIELD_MESSAGE = "Favor constructor injection and avoid field injection - "; | ||
public static final String NO_AUTOWIRED_FIELD_MESSAGE = "Favor constructor injection and avoid autowiring fields - "; | ||
public static final String NO_JODA_VIOLATION_MESSAGE = "Use Java8 Date API instead of Joda library"; | ||
public static final String NO_JAVA_UTIL_DATE_VIOLATION_MESSAGE = "Use Java8 java.time or java.util.GregorianCalendar or java.text.DateFormat to parse and format dates instead of java.util.Date library because they support internationalization better"; | ||
public static final String NO_JUNIT_IGNORE_VIOLATION_MESSAGE = "Tests shouldn't been ignored"; | ||
public static final String NO_JUNIT_IGNORE_WITHOUT_COMMENT_VIOLATION_MESSAGE = "Tests shouldn't been ignored without providing a comment explaining why"; | ||
|
||
private ArchUtils() { | ||
throw new UnsupportedOperationException(); | ||
} | ||
private ArchUtils() { | ||
throw new UnsupportedOperationException(); | ||
} | ||
|
||
public static JavaClasses importAllClassesInPackage(String path, String classFolder) { | ||
Path classesPath = Paths.get(path + classFolder); | ||
if (classesPath.toFile().exists()) { | ||
return new ClassFileImporter().importPath(classesPath); | ||
} | ||
return new ClassFileImporter().importPath(Paths.get(path)); | ||
} | ||
public static JavaClasses importAllClassesInPackage(String path, String classFolder) { | ||
Path classesPath = Paths.get(path + classFolder); | ||
if (classesPath.toFile().exists()) { | ||
return new ClassFileImporter().importPath(classesPath); | ||
} | ||
return new ClassFileImporter().importPath(Paths.get(path)); | ||
} | ||
|
||
public static boolean isJunitAssert(JavaClass javaClass) { | ||
public static boolean isJunitAssert(JavaClass javaClass) { | ||
|
||
String packageNameToCheck=new StringBuilder().append(javaClass.getPackageName()).append(PACKAGE_SEPARATOR).append(javaClass.getSimpleName()).toString(); | ||
String packageNameToCheck = new StringBuilder().append(javaClass.getPackageName()).append(PACKAGE_SEPARATOR) | ||
.append(javaClass.getSimpleName()).toString(); | ||
|
||
return JUNIT4_ASSERT_PACKAGE_NAME.equals(packageNameToCheck) || JUNIT5_ASSERT_PACKAGE_NAME.equals(packageNameToCheck); | ||
} | ||
return JUNIT4_ASSERT_PACKAGE_NAME.equals(packageNameToCheck) | ||
|| JUNIT5_ASSERT_PACKAGE_NAME.equals(packageNameToCheck); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
package com.societegenerale.commons.plugin.rules; | ||
|
||
import static com.societegenerale.commons.plugin.utils.ArchUtils.NO_JAVA_UTIL_DATE_VIOLATION_MESSAGE; | ||
import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.classes; | ||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.Assertions.assertThatCode; | ||
import static org.assertj.core.api.Assertions.catchThrowable; | ||
|
||
import org.junit.Test; | ||
|
||
import com.societegenerale.commons.plugin.rules.classesForTests.ObjectWithJava8TimeLib; | ||
import com.societegenerale.commons.plugin.rules.classesForTests.ObjectWithJavaTextDateFormat; | ||
import com.societegenerale.commons.plugin.rules.classesForTests.ObjectWithJavaUtilDateReferences; | ||
import com.societegenerale.commons.plugin.rules.classesForTests.ObjectWithJavaUtilGregorianCalendar; | ||
import com.tngtech.archunit.core.domain.JavaClasses; | ||
import com.tngtech.archunit.core.importer.ClassFileImporter; | ||
|
||
public class NoJavaUtilDateRuleTestTest { | ||
|
||
private JavaClasses classesUsingJavaUtilDateLibrary = new ClassFileImporter() | ||
.importClasses(ObjectWithJavaUtilDateReferences.class); | ||
|
||
private JavaClasses classesUsingJava8Library = new ClassFileImporter().importClasses(ObjectWithJava8TimeLib.class); | ||
private JavaClasses classesUsingJavaTextDateFormatLibrary = new ClassFileImporter() | ||
.importClasses(ObjectWithJavaTextDateFormat.class); | ||
private JavaClasses classesUsingJavaUtilGregorianCalendarLibrary = new ClassFileImporter() | ||
.importClasses(ObjectWithJavaUtilGregorianCalendar.class); | ||
|
||
@Test | ||
public void shouldCatchViolationsInStaticBlocksAndMemberFields() { | ||
|
||
Throwable validationExceptionThrown = catchThrowable(() -> { | ||
|
||
classes().should(NoJavaUtilDateRuleTest.notUseJavaUtilDate()).check(classesUsingJavaUtilDateLibrary); | ||
|
||
}); | ||
|
||
assertThat(validationExceptionThrown).hasMessageContaining(ObjectWithJavaUtilDateReferences.class.getName()) | ||
.hasMessageContaining(NO_JAVA_UTIL_DATE_VIOLATION_MESSAGE); | ||
} | ||
|
||
@Test(expected = Throwable.class) | ||
public void shouldThrowNoJavaUtilDateViolation() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the added value of that test compared to previous one ? It's same, no ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, I'll delete this second test. |
||
classes().should(NoJavaUtilDateRuleTest.notUseJavaUtilDate()).check(classesUsingJavaUtilDateLibrary); | ||
} | ||
|
||
@Test | ||
public void shouldNotThrowAnyViolation() { | ||
assertThatCode( | ||
() -> classes().should(NoJavaUtilDateRuleTest.notUseJavaUtilDate()).check(classesUsingJava8Library)) | ||
.doesNotThrowAnyException(); | ||
|
||
assertThatCode(() -> classes().should(NoJavaUtilDateRuleTest.notUseJavaUtilDate()) | ||
.check(classesUsingJavaTextDateFormatLibrary)).doesNotThrowAnyException(); | ||
|
||
assertThatCode(() -> classes().should(NoJavaUtilDateRuleTest.notUseJavaUtilDate()) | ||
.check(classesUsingJavaUtilGregorianCalendarLibrary)).doesNotThrowAnyException(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
package com.societegenerale.commons.plugin.rules.classesForTests; | ||
|
||
import java.text.DateFormat; | ||
|
||
public class ObjectWithJavaTextDateFormat { | ||
|
||
private DateFormat dateFormat; | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
package com.societegenerale.commons.plugin.rules.classesForTests; | ||
|
||
import java.util.Date; | ||
|
||
public class ObjectWithJavaUtilDateReferences { | ||
|
||
private Date date = new Date(0, 0, 0); | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
package com.societegenerale.commons.plugin.rules.classesForTests; | ||
|
||
import java.util.GregorianCalendar; | ||
|
||
public class ObjectWithJavaUtilGregorianCalendar { | ||
|
||
private GregorianCalendar cal; | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do it in 2 steps ? instead of collecting the items, you could directly a foreach and add the violations in events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great