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

feat: Add CtTry.addCatcherAt #4954

Merged
merged 8 commits into from
Oct 23, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
12 changes: 12 additions & 0 deletions src/main/java/spoon/reflect/code/CtTry.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,18 @@ public interface CtTry extends CtStatement, TemplateParameter<Void>, CtBodyHolde
@PropertySetter(role = CATCH)
<T extends CtTry> T addCatcher(CtCatch catcher);

/**
* Adds a catch block at the specified position in the <code>try</code> statement.
* Behaves similarly to {@link java.util.List#add(int, Object)}.
*
* @param position the position at which the <code>catcher</code> is to be inserted
* @param catcher the catch statement to be inserted
* @return this try statement
* @throws IndexOutOfBoundsException if the position is out of range (position < 0 || position > number of catchers)
*/
@PropertySetter(role = CATCH)
CtTry addCatcherAt(int position, CtCatch catcher);

/**
* Removes a catch block.
*/
Expand Down
12 changes: 9 additions & 3 deletions src/main/java/spoon/support/reflect/code/CtTryImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,22 @@ public <T extends CtTry> T setCatchers(List<CtCatch> catchers) {

@Override
public <T extends CtTry> T addCatcher(CtCatch catcher) {
addCatcherAt(catchers.size(), catcher);
return (T) this;
}

@Override
public CtTry addCatcherAt(int position, CtCatch catcher) {
if (catcher == null) {
return (T) this;
return this;
}
if (catchers == CtElementImpl.<CtCatch>emptyList()) {
catchers = new ArrayList<>(CATCH_CASES_CONTAINER_DEFAULT_CAPACITY);
}
catcher.setParent(this);
getFactory().getEnvironment().getModelChangeListener().onListAdd(this, CATCH, this.catchers, catcher);
catchers.add(catcher);
return (T) this;
catchers.add(position, catcher);
return this;
}

@Override
Expand Down
52 changes: 51 additions & 1 deletion src/test/java/spoon/test/trycatch/TryCatchTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.Set;

import org.apache.commons.lang3.StringUtils;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import spoon.Launcher;
Expand All @@ -36,7 +37,6 @@
import spoon.reflect.code.CtResource;
import spoon.reflect.code.CtTry;
import spoon.reflect.code.CtTryWithResource;
import spoon.reflect.code.CtVariableRead;
import spoon.reflect.declaration.CtClass;
import spoon.reflect.declaration.CtMethod;
import spoon.reflect.declaration.CtVariable;
Expand All @@ -56,11 +56,13 @@

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.collection.IsIterableContainingInOrder.contains;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static spoon.testing.utils.ModelUtils.build;
Expand Down Expand Up @@ -442,4 +444,52 @@ public void testTryWithVariableAsResource() {
tryStmt.removeResource(ctResource);

}

@Nested
class AddCatcherAt {
@Test
void addsCatcherAtTheSpecifiedPosition() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: This test really tests three separate cases in one go:

  1. Inserting into an empty list.
  2. Inserting at the beginning of a non-empty list.
  3. Inserting at a position that is neither first nor last.

Given that this functionality is neither complicated nor critical to Spoon, that's probably fine. But it always bears thinking about how much is tested in any single test. The fact that we never insert at the end of a non-empty list also isn't a big deal considering what the implementation looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it always bears thinking about how much is tested in any single test.

Right. Ideally, we should test only one case per unit test. I will keep this in mind. We can skip refactoring for this feature because, like you said, it is not that critical and is rather a very simple feature.

The fact that we never insert at the end of a non-empty list also isn't a big deal considering what the implementation looks like.

Tests of addCatcher would already test that so we do not need to think about them I guess.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tests of addCatcher would already test that so we do not need to think about them I guess.

Precisely, "considering what the implementation looks like" :). White box testing has merits in that we can omit redundant testing, but it has drawbacks to match (e.g. creating tests based on the implementation rather than the specification).

// contract: the catcher should be added at the specified position
// arrange
Factory factory = createFactory();

CtTry tryStatement = factory.createTry();

CtCatch first = createCatch(factory, IOException.class);
CtCatch second = createCatch(factory, ExceptionInInitializerError.class);
CtCatch third = createCatch(factory, NoSuchMethodException.class);

// act
tryStatement
.addCatcherAt(0, third)
.addCatcherAt(0, first)
.addCatcherAt(1, second);

// assert
assertThat(tryStatement.getCatchers(), contains(first, second, third));
}

@Test
void throwsOutOfBoundsException_whenPositionIsOutOfBounds() {
// contract: `addCatcherAt` should throw an out-of-bounds exception when the specified position is out of
// bounds of the catcher collection
SirYwell marked this conversation as resolved.
Show resolved Hide resolved

// arrange
Factory factory = createFactory();

CtTry tryStatement = factory.createTry();
CtCatch catcherAtWrongPosition = createCatch(factory, Exception.class);

// act & assert
assertThrows(IndexOutOfBoundsException.class,
() -> tryStatement.addCatcherAt(1, catcherAtWrongPosition));
}

private <T extends Throwable> CtCatch createCatch(Factory factory, Class<T> typeToCatch) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it type stricter by replacing T with <T extends Throwable>

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

CtTypeReference<T> typeReference = factory.Type().createReference(typeToCatch);
return factory.createCatch().setParameter(
factory.createCatchVariable().setType(typeReference)
);
}
}
}