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

review: refactor: Update EnumsTest to JUnit 5 #4138

Merged
merged 8 commits into from
Oct 4, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/main/java/spoon/support/reflect/CtExtendedModifier.java
Original file line number Diff line number Diff line change
Expand Up @@ -93,4 +93,11 @@ public ElementSourceFragment getOriginalSourceFragment() {
return ElementSourceFragment.NO_SOURCE_FRAGMENT;
}
}

@Override
public String toString() {
return "CtExtendedModifier{"
+ "implicit=" + implicit
+ ", kind=" + kind + '}';
}
Copy link
Collaborator

@slarse slarse Oct 4, 2021

Choose a reason for hiding this comment

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

This is a production code change, it should not be necessary to migrate the tests to JUnit5. Where exactly is this utilized?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Failing tests where modifiers don't match were hard (impossible) to read. I can move this change to the bug fixing PR too, there it would be more related probably.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I see. That's a fine enough justification for that.

}
18 changes: 18 additions & 0 deletions src/test/java/spoon/test/SpoonTestHelpers.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@
*/
package spoon.test;

import org.hamcrest.Matcher;
import spoon.Launcher;
import spoon.testing.matchers.ContentEqualsMatcher;
import spoon.testing.matchers.RegexFindMatcher;
import spoon.metamodel.Metamodel;
import spoon.reflect.CtModel;
import spoon.reflect.code.CtBlock;
Expand All @@ -30,8 +33,12 @@
import spoon.support.UnsettableProperty;
import spoon.support.compiler.VirtualFile;

import javax.annotation.RegEx;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.regex.Pattern;

import static org.junit.Assume.assumeFalse;

Expand Down Expand Up @@ -183,4 +190,15 @@ public static String wrapLocal(String localDeclarationSnippet) {
" }\n" +
"}";
}

/** @see RegexFindMatcher */
public static Matcher<String> containsRegexMatch(@RegEx String regex) {
return new RegexFindMatcher(Pattern.compile(regex));
}

/** @see ContentEqualsMatcher */
@SafeVarargs
public static <T extends Collection<E>, E> Matcher<T> contentEquals(E... elements) {
return new ContentEqualsMatcher<>(Arrays.asList(elements));
}
}
185 changes: 105 additions & 80 deletions src/test/java/spoon/test/enums/EnumsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,136 +16,139 @@
*/
package spoon.test.enums;

import com.google.common.collect.Streams;
import org.apache.commons.lang3.StringUtils;
import org.junit.Test;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.ArgumentsProvider;
import org.junit.jupiter.params.provider.ArgumentsSource;
import spoon.Launcher;
import spoon.reflect.CtModel;
import spoon.reflect.code.CtBlock;
import spoon.reflect.code.CtExpression;
import spoon.reflect.code.CtStatement;
import spoon.reflect.declaration.CtAnnotationType;
import spoon.reflect.declaration.CtEnum;
import spoon.reflect.declaration.CtEnumValue;
import spoon.reflect.declaration.CtField;
import spoon.reflect.declaration.CtMethod;
import spoon.reflect.declaration.CtType;
import spoon.reflect.declaration.CtField;
import spoon.reflect.declaration.ModifierKind;
import spoon.reflect.factory.Factory;
import spoon.reflect.visitor.DefaultJavaPrettyPrinter;
import spoon.reflect.visitor.filter.TypeFilter;
import spoon.support.reflect.CtExtendedModifier;
import spoon.test.SpoonTestHelpers;
import spoon.test.annotation.AnnotationTest;
import spoon.test.enums.testclasses.Burritos;
import spoon.test.enums.testclasses.Foo;
import spoon.test.enums.testclasses.EnumWithMembers;
import spoon.test.enums.testclasses.NestedEnums;
import spoon.test.enums.testclasses.Regular;
import spoon.test.enums.testclasses.EnumWithMembers;
import spoon.testing.utils.ModelUtils;

import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
import static spoon.testing.utils.ModelUtils.build;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static org.hamcrest.CoreMatchers.hasItem;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static spoon.test.SpoonTestHelpers.containsRegexMatch;
import static spoon.test.SpoonTestHelpers.contentEquals;
import static spoon.testing.utils.ModelUtils.build;

public class EnumsTest {

@Test
public void testModelBuildingEnum() throws Exception {
void testModelBuildingEnum() throws Exception {
// contract: an enum built by spoon equals the real enum in following aspects:
// - the simple name
// - the amount of enum values
// - the order of enum values
// - the amount of overall fields
CtEnum<Regular> enumeration = build("spoon.test.enums.testclasses", "Regular");
assertEquals("Regular", enumeration.getSimpleName());
assertEquals(3, Regular.values().length);
assertEquals(3, enumeration.getEnumValues().size());
assertEquals("A", enumeration.getEnumValues().get(0).getSimpleName());
assertEquals(5, enumeration.getFields().size());
assertThat(enumeration.getSimpleName(), is("Regular"));
assertThat(enumeration.getEnumValues().size(), is(Regular.values().length));
assertThat(map(CtEnumValue::getSimpleName, enumeration.getEnumValues()), is(Arrays.asList("A", "B", "C")));
assertThat(enumeration.getFields().size(), is(5));
}

@Test
public void testAnnotationsOnEnum() {
final Launcher launcher = new Launcher();
launcher.run(new String[]{
"-i", "./src/test/java/spoon/test/enums/testclasses",
"-o", "./target/spooned"
});

final CtEnum<?> foo = (CtEnum) launcher.getFactory().Type().get(Foo.class);
assertEquals(1, foo.getFields().size());
assertEquals(1, foo.getFields().get(0).getAnnotations().size());
void testAnnotationsOnEnum() throws Exception {
// contract: an annotation on an enum value is represented in the spoon model
final CtEnum<?> foo = build("spoon.test.enums.testclasses", "Foo");
assertThat(foo.getEnumValues().size(), is(1));
CtEnumValue<?> value = foo.getEnumValues().get(0);
assertThat(value.getAnnotations().size(), is(1));
CtAnnotationType<?> annotation = (CtAnnotationType<?>) foo.getFactory().Annotation().<Deprecated>get(Deprecated.class);
assertThat(map(AnnotationTest::getActualClassFromAnnotation, value.getAnnotations()), hasItem(Deprecated.class));
assertSame(Deprecated.class, AnnotationTest.getActualClassFromAnnotation(
foo.getFields().get(0).getAnnotations().get(0)));
assertEquals(
"public enum Foo {" + DefaultJavaPrettyPrinter.LINE_SEPARATOR + DefaultJavaPrettyPrinter.LINE_SEPARATOR
+ " @java.lang.Deprecated"
+ DefaultJavaPrettyPrinter.LINE_SEPARATOR + " Bar;}",
foo.toString());
// finding with a regex to avoid printer-specific output
assertThat(foo.prettyprint(), containsRegexMatch("@(java\\.lang\\.)?Deprecated\\W+Bar"));
}

private static <I, O> List<O> map(Function<I, O> function, List<I> input) {
return input.stream().map(function).collect(Collectors.toList());
}

@Test
public void testEnumWithoutField() throws Exception {
void testEnumWithoutValue() throws Exception {
// contract: an enum without values contains a ; before any other members
final Factory factory = build(Burritos.class);
final CtType<Burritos> burritos = factory.Type().get(Burritos.class);
assertEquals("public enum Burritos {" + DefaultJavaPrettyPrinter.LINE_SEPARATOR //
+ " ;" + DefaultJavaPrettyPrinter.LINE_SEPARATOR + DefaultJavaPrettyPrinter.LINE_SEPARATOR //
+ " public static void m() {" + DefaultJavaPrettyPrinter.LINE_SEPARATOR //
+ " }" + DefaultJavaPrettyPrinter.LINE_SEPARATOR //
+ "}", burritos.toString());
assertThat(burritos.prettyprint(), containsRegexMatch("Burritos \\{\\W+;"));
}

@Test
public void testGetAllMethods() throws Exception {
void testGetAllMethods() throws Exception {
// contract: getAllMethods also returns the methods of Enum
final Factory factory = build(Burritos.class);
final CtType<Burritos> burritos = factory.Type().get(Burritos.class);
CtMethod name = factory.Core().createMethod();
CtMethod<String> name = factory.Core().createMethod();
name.setSimpleName("name"); // from Enum
name.setType(factory.Type().createReference(String.class));
assertTrue(burritos.hasMethod(name));
assertTrue(burritos.getAllMethods().contains(name));
// this does not work due to the SignatureBasedSortedSet violating the Set contract
// assertThat(burritos.getAllMethods(), hasItem(name));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is noise for this test, and it's very unlikely to be removed if SignatureBasedSortedSet is fixed. I'd remove it.

}

@Test
public void testNestedPrivateEnumValues() throws Exception {
@ParameterizedTest
@ArgumentsSource(NestedEnumTypeProvider.class)
void testEnumValueModifiers(CtEnum<?> type, CtExtendedModifier visibility) {
// contract: enum values have correct modifiers
CtType<?> ctClass = ModelUtils.buildClass(NestedEnums.class);
{
CtEnum<?> ctEnum = ctClass.getNestedType("PrivateENUM");
assertEquals(asSet(ModifierKind.PRIVATE), ctEnum.getModifiers());
assertEquals(asSet(ModifierKind.PRIVATE, ModifierKind.STATIC, ModifierKind.FINAL), ctEnum.getField("VALUE").getModifiers());
}
{
CtEnum<?> ctEnum = ctClass.getNestedType("PublicENUM");
assertEquals(asSet(ModifierKind.PUBLIC), ctEnum.getModifiers());
assertEquals(asSet(ModifierKind.PUBLIC, ModifierKind.STATIC, ModifierKind.FINAL), ctEnum.getField("VALUE").getModifiers());
}
{
CtEnum<?> ctEnum = ctClass.getNestedType("ProtectedENUM");
assertEquals(asSet(ModifierKind.PROTECTED), ctEnum.getModifiers());
assertEquals(asSet(ModifierKind.PROTECTED, ModifierKind.STATIC, ModifierKind.FINAL), ctEnum.getField("VALUE").getModifiers());
}
{
CtEnum<?> ctEnum = ctClass.getNestedType("PackageProtectedENUM");
assertEquals(asSet(), ctEnum.getModifiers());
assertEquals(asSet(ModifierKind.STATIC, ModifierKind.FINAL), ctEnum.getField("VALUE").getModifiers());
if (visibility != null) {
assertThat(type.getField("VALUE").getExtendedModifiers(), contentEquals(
// TODO this is wrong, the field should be public
new CtExtendedModifier(visibility.getKind(), true),
new CtExtendedModifier(ModifierKind.STATIC, true),
new CtExtendedModifier(ModifierKind.FINAL, true)
));
} else {
assertThat(type.getField("VALUE").getExtendedModifiers(), contentEquals(
// TODO this is wrong, the field should be public (and then the if else,
// the visibility param and the Streams#zip can be removed)
new CtExtendedModifier(ModifierKind.STATIC, true),
new CtExtendedModifier(ModifierKind.FINAL, true)
));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is an improvement over the original test. While the original has the problem that it's 4 tests in one, it's very easy to read. The refactored version has 2 mutually exclusive branching paths (in general, avoid branches in tests) and also requires one to understand the non-trivial parameterization.

I favor the original test, it's much easier to grasp. I'm guessing you have a plan here for when you fix the modifier bug, but that's not a concern of this PR. Could you revert to the original? Feel free to reintroduce the parameterized test if it actually simplifies things in a future PR.

}
}

private <T> Set<T> asSet(T... values) {
return new HashSet<>(Arrays.asList(values));
}

@Test
public void testPrintEnumValues() throws IOException {
void testPrintEnumValues() throws IOException {
// contract: enum values constructor calls are correctly interpreted as implicit or not
// TODO this test is incomplete, it does not cover anonymous enum constants
Launcher launcher = new Launcher();
launcher.addInputResource("./src/test/java/spoon/test/comment/testclasses/EnumClass.java");
launcher.setSourceOutputDirectory("./target/test-enum");
Expand All @@ -172,20 +175,20 @@ public void testPrintEnumValues() throws IOException {
}

@Test
public void testEnumValue() {
void testEnumValue() {
// contract: constructorCall on enum values should be implicit if they're not declared

// TODO this test is incomplete, it does not cover anonymous enum constants
Launcher launcher = new Launcher();
launcher.addInputResource("./src/test/java/spoon/test/comment/testclasses/EnumClass.java");
CtModel model = launcher.buildModel();

List<CtEnumValue> enumValues = model.getElements(new TypeFilter<>(CtEnumValue.class));
List<CtEnumValue<?>> enumValues = model.getElements(new TypeFilter<>(CtEnumValue.class));

assertEquals(4, enumValues.size());
assertThat(enumValues.size(), is(4));

for (int i = 0; i < 3; i++) {
CtEnumValue ctEnumValue = enumValues.get(i);
CtExpression defaultExpression = ctEnumValue.getDefaultExpression();
CtEnumValue<?> ctEnumValue = enumValues.get(i);
CtExpression<?> defaultExpression = ctEnumValue.getDefaultExpression();

if (i != 2) {
assertTrue(defaultExpression.isImplicit());
Expand All @@ -196,12 +199,12 @@ public void testEnumValue() {
}

@Test
public void testEnumMembersModifiers() throws Exception {
void testEnumMembersModifiers() throws Exception {
// contract: enum members should have correct modifiers
final Factory factory = build(EnumWithMembers.class);
CtModel model = factory.getModel();

CtField lenField = model.getElements(new TypeFilter<>(CtField.class)).stream()
CtField<?> lenField = model.getElements(new TypeFilter<>(CtField.class)).stream()
.filter(p -> "len".equals(p.getSimpleName()))
.findFirst().get();

Expand All @@ -212,7 +215,7 @@ public void testEnumMembersModifiers() throws Exception {
assertFalse(lenField.isProtected());
}

@org.junit.jupiter.api.Test
@Test
void testLocalEnumExists() {
// contract: local enums and their members are part of the model
String code = SpoonTestHelpers.wrapLocal(
Expand All @@ -236,4 +239,26 @@ void testLocalEnumExists() {
assertThat(enumType.getEnumValues().size(), is(2));
assertThat(enumType.getMethods().size(), is(1));
}

static class NestedEnumTypeProvider implements ArgumentsProvider {
private final CtType<?> ctClass;

NestedEnumTypeProvider() throws Exception {
this.ctClass = ModelUtils.buildClass(NestedEnums.class);
}

@Override
public Stream<? extends Arguments> provideArguments(ExtensionContext context) throws Exception {
Stream<CtEnum<?>> types = Stream.of("Private", "PackageProtected", "Protected", "Public")
.map(s -> s + "ENUM")
.map(ctClass::getNestedType);
//noinspection UnstableApiUsage
return Streams.zip(types, Stream.of(
new CtExtendedModifier(ModifierKind.PRIVATE),
null, // package private modifier
new CtExtendedModifier(ModifierKind.PROTECTED),
new CtExtendedModifier(ModifierKind.PUBLIC)
), Arguments::of);
}
}
}
39 changes: 39 additions & 0 deletions src/test/java/spoon/testing/matchers/ContentEqualsMatcher.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package spoon.testing.matchers;

import org.hamcrest.Description;
import org.hamcrest.TypeSafeMatcher;

import java.util.Collection;
import java.util.HashSet;

/**
* A matcher that compares the contents of two collections.
*
* The contents of two collections are considered as equal
* if both contain the exact same elements. Order and frequency
* of elements is <b>not</b> considered.
*
* @param <T> the type of the collection to check
* @param <E> the element type stored in the collection
*/
public class ContentEqualsMatcher<T extends Collection<E>, E> extends TypeSafeMatcher<T> {
private final Collection<E> elements;

public ContentEqualsMatcher(Collection<E> elements) {
this.elements = elements;
}

@Override
protected boolean matchesSafely(T item) {
HashSet<E> expected = new HashSet<>(elements);
HashSet<E> actual = new HashSet<>(item);
return expected.equals(actual);
}

@Override
public void describeTo(Description description) {
description
.appendText("contains only but all of ")
.appendValue(elements);
}
}
Loading