Expand missing testing for PropertyCacheFile tests #3650

Closed
rnveach opened this Issue Dec 12, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Dec 12, 2016

Taken from #3594.

The reason why our tests passed but our coverage failed is because we are calling methods and not verifying any type of result. There were only some minor comments that eluded to what was being tested.
Example:
The test before the fix:

@Test
public void testCacheIoExceptionWhenReadingExternalResource() throws Exception {
final SuppressionFilter mock = PowerMockito.mock(SuppressionFilter.class);
final Set<String> mockResourceLocations = new HashSet<>(1);
mockResourceLocations.add("http://mock.sourceforge.net/suppressions_none.xml");
when(mock.getExternalResourceLocations()).thenReturn(mockResourceLocations);
final DefaultConfiguration checkerConfig = new DefaultConfiguration("checkstyle_checks");
checkerConfig.addAttribute("cacheFile", temporaryFolder.newFile().getPath());
final Checker checker = new Checker();
checker.addFilter(mock);
checker.setModuleClassLoader(Thread.currentThread().getContextClassLoader());
checker.configure(checkerConfig);
final String pathToEmptyFile = temporaryFolder.newFile("file.java").getPath();
final String[] expected = CommonUtils.EMPTY_STRING_ARRAY;
verify(checker, pathToEmptyFile, pathToEmptyFile, expected);
// One more time to use cahce.
verify(checker, pathToEmptyFile, pathToEmptyFile, expected);
}

The test after the fix:
@SuppressWarnings("unchecked")
@Test
public void testNonExistingResource() throws IOException {
final Configuration config = new DefaultConfiguration("myName");
final String filePath = temporaryFolder.newFile().getPath();
final PropertyCacheFile cache = new PropertyCacheFile(config, filePath);
// create cache with one file
cache.load();
final String myFile = "myFile";
cache.put(myFile, 1);
final String hash = cache.get(PropertyCacheFile.CONFIG_HASH_KEY);
assertNotNull(hash);
mockStatic(ByteStreams.class);
when(ByteStreams.toByteArray(any(BufferedInputStream.class)))
.thenThrow(IOException.class);
// apply new external resource to clear cache
final Set<String> resources = new HashSet<>();
final String resource = "/com/puppycrawl/tools/checkstyle/java.header";
resources.add(resource);
cache.putExternalResources(resources);
assertFalse(cache.isInCache(myFile, 1));
assertFalse(cache.isInCache(resource, 1));
}

There are more tests like this for the cache in CheckerTest that could cause the same type of failure. These tests should be fixed to actually test their results to make sure they are working correctly.
Some examples:

verify(checker, pathToEmptyFile, expected);
// Once again to invalidate cache because in the second run IOException will happen
// on different line for DummyFileSetCheck and it will change the content
verify(checker, pathToEmptyFile, expected);

verify(checker, pathToEmptyFile, expected);
// One more time to use cache.
verify(checker, pathToEmptyFile, expected);

verify(checker, pathToEmptyFile, expected);
// Change a list of external resources which are used by the check
check.setSecondExternalResourceLocation("checks" + File.separator
+ "imports" + File.separator + "import-control_one-re.xml");
verify(checker, pathToEmptyFile, expected);

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 12, 2016

Member

Pitest should also catch all tests that does not care about result

Member

romani commented Dec 12, 2016

Pitest should also catch all tests that does not care about result

@rnveach rnveach added the medium label Mar 2, 2017

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Mar 26, 2017

Contributor

I'm on it.

Contributor

MEZk commented Mar 26, 2017

I'm on it.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Mar 26, 2017

Contributor

@rnveach
Not only the tests for PropertyCacheFile, but also the tests for Checker itself assert nothing. I think that they also need to be fixed.

More examples:

public void testFinishLocalSetupFullyInitialized() throws Exception {

Contributor

MEZk commented Mar 26, 2017

@rnveach
Not only the tests for PropertyCacheFile, but also the tests for Checker itself assert nothing. I think that they also need to be fixed.

More examples:

public void testFinishLocalSetupFullyInitialized() throws Exception {

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 27, 2017

Member

I am fine with making any tests better.

Member

rnveach commented Mar 27, 2017

I am fine with making any tests better.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Mar 30, 2017

Contributor

Not only the tests for PropertyCacheFile, but also the tests for Checker itself assert nothing. I think that they also need to be fixed.

Will be addressed in the scope of #4128.

Contributor

MEZk commented Mar 30, 2017

Not only the tests for PropertyCacheFile, but also the tests for Checker itself assert nothing. I think that they also need to be fixed.

Will be addressed in the scope of #4128.

MEZk added a commit to MEZk/checkstyle that referenced this issue Mar 30, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Apr 1, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Apr 2, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Apr 15, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Apr 15, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Apr 15, 2017

MEZk added a commit to MEZk/checkstyle that referenced this issue Apr 16, 2017

romani added a commit that referenced this issue Apr 16, 2017

@romani romani added this to the 7.7 milestone Apr 16, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 16, 2017

Member

fix is merged.

if smt is left, it will be addressed during #3746 .

Member

romani commented Apr 16, 2017

fix is merged.

if smt is left, it will be addressed during #3746 .

@romani romani unassigned MEZk May 1, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 25, 2017

Member

I am closing this since fix was merged and anything else found will be done as part of GSoC issues

Member

rnveach commented May 25, 2017

I am closing this since fix was merged and anything else found will be done as part of GSoC issues

@rnveach rnveach closed this May 25, 2017

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