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.

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

test: improve tests for NameFilter class. #3975

Merged
merged 8 commits into from
Jun 11, 2021
Merged

test: improve tests for NameFilter class. #3975

Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1947cfc
add testNameFilterThrowsException
Rohitesh-Kumar-Jain Jun 4, 2021
c68f244
add more assertions and remove the redundant one
Rohitesh-Kumar-Jain Jun 6, 2021
5bd6499
remove separte test case which is now redundant
Rohitesh-Kumar-Jain Jun 6, 2021
3fbd091
add contracts to the assertions
Rohitesh-Kumar-Jain Jun 8, 2021
ae2e352
break assertions into separate tests
Rohitesh-Kumar-Jain Jun 9, 2021
f2d36c3
improve contract and remove rouge parentheses
Rohitesh-Kumar-Jain Jun 10, 2021
422365b
change contract of the testNameFilter
Rohitesh-Kumar-Jain Jun 10, 2021
e039a75
improve contract for testNameFilterGetType
Rohitesh-Kumar-Jain Jun 10, 2021
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
35 changes: 34 additions & 1 deletion src/test/java/spoon/test/filters/FilterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,46 @@ public void setup() throws Exception {

@Test
public void testNameFilter() throws Exception {
// contract: legacy NameFilter is tested and works
// contract: the constructor of the class NameFilter creates and initialises an object.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This contract is also not correct, this test is not for NameFilter's constructor. The test verifies that NameFilter finds the expected amount of elements when filtering with it (it's a very weak test).

The original comment was technically correct, it was just pointless. But you don't have to change this contract when adding your other test methods, as you don't touch this test method. I'm guessing this is just left over from when you modified this method.

Either fix this comment such that it's correct, or revert to the original contract.


CtClass<?> foo = factory.Package().get("spoon.test.filters.testclasses").getType("Foo");
assertEquals("Foo", foo.getSimpleName());
Rohitesh-Kumar-Jain marked this conversation as resolved.
Show resolved Hide resolved
List<CtNamedElement> elements = foo.getElements(new NameFilter<>("i"));
assertEquals(1, elements.size());
}

@Test
void testNameFilterMatchesReturnsTrueForMatchingName() {
// contract: NameFilter.matches should return true for an element with a matching name

// arrange
String name = "someNiceName";
NameFilter<CtNamedElement> nameFilter = new NameFilter<>(name);
CtNamedElement elemWithMatchingName = new Launcher().getFactory().createLocalVariable();
elemWithMatchingName.setSimpleName(name);

// act
boolean matches = nameFilter.matches(elemWithMatchingName);

// assert
assertTrue(matches);
}
Rohitesh-Kumar-Jain marked this conversation as resolved.
Show resolved Hide resolved

@Test()
Rohitesh-Kumar-Jain marked this conversation as resolved.
Show resolved Hide resolved
public void testNameFilterThrowsExceptionForNull() {
// contract: NameFilter constructor should throw IllegalArgumentException when passed null

assertThrows(IllegalArgumentException.class, () -> new NameFilter<>(null));
}

@Test()
public void testNameFilterGetType() {
// contract: getType method returns the object of class NameFilter
Copy link
Collaborator

Choose a reason for hiding this comment

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

This contract is not correct, the NameFilter.getType() is not tested to return a NameFilter object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should the contract be like this : getType method returns the type of class which is begin used ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no one answer to what a contract should say, but it cannot be incorrect. What the getType method actually is supossed to return is the CtNamedElement class, which is not an object of class NameFilter, and thus the comment here is explicitly incorrect.

The contract comment should state in human-readable terms what the test is supposed to verify. One potential contract here would be: // contract: NameFilter.getType() should return the CtNamedElement class

A less specific one would be: // contract: NameFilter.getType() should return the correct class, where the test itself then specifies that correct class.

Note that we don't usually test getters directly, because they are trivial pieces of code, and so the test and its contract here necessarily become a bit contrived.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that we don't usually test getters directly, because they are trivial pieces of code

I have one question, so I can skip adding tests for getter like getFactory() in this class ?

Copy link
Collaborator

@slarse slarse Jun 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I have one question, so I can skip adding tests for getter like getFactory() in this class ?

Directly testing that would be fairly far down the priority list, yes. Note however that this isn't a pure getter in the sense that it doesn't just return a static value or a field of the class, but actually calls some other method. In that sense, it's not pointless to test it, but it would for example be much more important to thoroughly test spoon.template.Substitution.getFactory (the method that's delegated to), as that method is more complex. Overall, the Substitution class looks fairly poorly tested when looking at the Coveralls report, so perhaps that'd be a good target for you!


NameFilter<CtNamedElement> nameFilter = new NameFilter<>("i");
assertEquals(CtNamedElement.class, nameFilter.getType());
}

@Test
public void testFilters() {
CtClass<?> foo = factory.Package().get("spoon.test.filters.testclasses").getType("Foo");
Expand Down