Failure of Bug468PerformanceTest.test

[fedejeanne]

Failed in this build of #704

org.eclipse.core.tests.internal.properties.Bug468PerformanceTest.test -- Time elapsed: 122.0 s <<< FAILURE!
junit.framework.AssertionFailedError: The expected max time(ms): 60000, actual time(ms): 70916
	at junit.framework.Assert.fail(Assert.java:57)
	at junit.framework.Assert.assertTrue(Assert.java:22)
	at junit.framework.TestCase.assertTrue(TestCase.java:192)
	at org.eclipse.core.tests.internal.properties.Bug468PerformanceTest.test(Bug468PerformanceTest.java:163)

[jukzi]

i got The expected max time(ms): 60000, actual time(ms): 96100. please verify there is no performance regression. If none just increase timeout

[jukzi]

@lathapatil @iloveeclipse test was added for #468

[HeikoKlare]

There already is a PR for increasing the timeout: #852
The time taken by the test differs heavily between test runs, maybe also because of hardware utilization. Right now we have several builds running to get all pending PRs merged, so maybe several jobs are running on the same hardware affecting the times taken by the test.

And there was also some discussion in the PR for increasing the timeout on how to replace that performance test with a functional test, since no performance test is actually required here (and it obviously takes quite some build time, even if timeout is set properly). But as a quick fix I would like to merge the timeout change to have more stable builds and replace the test afterwards.

[HeikoKlare]

Temporary disablement proposed in #891.

One note for the test reimplementation: The test is currently written on JUnit 3, which is pretty outdated. When rewriting the test, please write it for JUnit 4/5 instead.

[HeikoKlare]

For documentation of discussion in PR #852 on replacement of the performance test with a functional regression test:

If I see correctly, this is supposed to be a regression test for #468. This bug is about unnecessary tree traversal with delete calls on every element. Wouldn't it be sufficient to write a regression test for the behavior rather than for its performance? I.e., replace this performance test by one that validates that delete is only called on the the root IFile in the addressed scenario, but not for elements deeper in the tree?

See #852 (comment)

[lathapatil]

replacement of the performance test with a functional regression test:

Is it expected to have below kind of testcase ? If yes , I am stuck with issues in completing the case ..

I have written it to verify for given IFile resource deleteProperties method is called with depth Zero

@Mock
PropertyManager2 manager;
@Test
	public void test() throws CoreException {

		IFolder tempFolder = this.project.getFolder(TEMP_FOLDER_NAME);
		IFile fileToBeDeleted = tempFolder.getFile(TO_BE_DELETED_FILE_NAME_PREFIX + 0);
		
			Mockito.doCallRealMethod().when(manager).deleteResource(fileToBeDeleted);
			
			verify(manager).deleteProperties(fileToBeDeleted, 0);

	}

Facing an error

Wanted but not invoked:
propertyManager2.deleteProperties(
L/test_TestProject/temp/file_0,
0
);
-> at org.eclipse.core.internal.properties.PropertyManager2.deleteProperties(PropertyManager2.java:113)
Actually, there were zero interactions with this mock.

And also tried with Argument capture

@Mock
	PropertyManager2 manager;
	@Captor
	ArgumentCaptor<IResource> resourceArgCaptor;
	@Captor
	ArgumentCaptor<Integer> depthArgCapture;
	@Test
	public void test() throws CoreException {

		IFolder tempFolder = this.project.getFolder(TEMP_FOLDER_NAME);
		IFile fileToBeDeleted = tempFolder.getFile(TO_BE_DELETED_FILE_NAME_PREFIX + 0);
		
		manager.deleteResource(fileToBeDeleted);

		Mockito.verify(manager).deleteProperties(resourceArgCaptor.capture(), depthArgCapture.capture());
		Integer expectedDepth = 0;
		assertEquals(expectedDepth, depthArgCapture.getValue());
		}

But, Facing an error here too

Wanted but not invoked:
manager.deleteProperties(
,

);
-> at org.eclipse.core.internal.properties.PropertyManager2.deleteProperties(PropertyManager2.java:113)

However, there was exactly 1 interaction with this mock:
manager.deleteResource(
L/test_TestProject/temp/file_0

[lathapatil]

Is it expected to have below kind of testcase ? If yes , I am stuck with issues in completing the case ..

I am able to resolve the issues . @HeikoKlare I just need confirmation if this is the kind of testcase expected so that I can go for new PR

[HeikoKlare]

Yes, that would be the idea for such a test case.