Failure of Bug468PerformanceTest.test #715

[lathapatil]

Failure of the testcase in Mac OS only.
Fix provided as such performance time limit is increased to 75 seconds from 60 seconds only for Mac OS. The testcase took around 71 seconds for Mac OS in the build
(https://github.com/eclipse-platform/eclipse.platform/actions/runs/6263790505/job/17008968379?pr=704)

Fixes #715

[lathapatil]

@fedejeanne could you please review the changes ?

[github-actions]

Test Results

   617 files     617 suites   38m 54s ⏱️
 3 946 tests  3 923 ✅  22 💤 1 ❌
12 295 runs  12 133 ✅ 161 💤 1 ❌

For more details on these failures, see this check.

Results for commit c1a6dd1.

♻️ This comment has been updated with latest results.

[fedejeanne]

@lathapatil I haven't been involved in the fix of the original issue (#468), maybe ask @jukzi / @iloveeclipse ?

FWIW this PR seems reasonable since the performance problem has been fixed by #468 and the test is rather new, so adapting the timeout makes sense. I'm curious though:

1. Why is the performance so bad in Mac? Is there another underlying issue?
2. Is the performance constant (in all Platforms) or does it vary on each execution of the test?

One last remark

Performance tests are always hardware-dependent so I would approach this in a different way: if possible, instead of testing the performance (i.e. the total time) I would test the performance improvement i.e. you could change the test to try and test either:
a. Total time of "the old way" > total time of "the new way" or (if not possible)
b. Total visits (from PropertyManager2) of "the old way" > total visits of "the new way"

If the previous approaches are not practicable then I would simply test that "the new way" produces the same result as "the old way" i.e. that all files are gone. This one wouldn't be a performance test but a "normal" test to show that nothing broke with the fix.

[HeikoKlare]

Thanks for working on a fix for that quite often failing test!

I only have minor comments that are mainly due to the fix conforming to the style of the existing code, while I would propose to apply some simple improvements to the code when extending it anyway.

[fedejeanne]

I've been thinking about it over the weekend and I found another reason not to add a performance test in this class: since this test is hardware dependent then it's bound to become obsolete over time:

1. If the hardware becomes too fast then "the old way" would probably also pass this test --> The test becomes obsolete
2. If the hardware is too slow then "the new way" would fail this test even though it's still faster than "the old way" --> The maxTimes need to be adapted again.
3. If 2. happens very often then at some point 1. could happen: hardware upgrade --> the test becomes obsolete

This is more of a general problem with any kind of performance test though, not just this test.

All in all, I would either remove this test or, if possible, rewrite as a benchmark test ("old" vs "new")

[lathapatil]

All in all, I would either remove this test or, if possible, rewrite as a benchmark test ("old" vs "new")

Thanks @fedejeanne for your valuable input . I do agree with your point that asserting against a constant time might not be proper way of testing the fix.

I would rather try to check if I can re write the case to check "old way of total time" > "New way of total time"

[HeikoKlare]

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?

[fedejeanne]

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?

That would be in fact my favorite option :)
That's what I meant in my first comment when I wrote:

b. Total visits (from PropertyManager2) of "the old way" > total visits of "the new way"

[HeikoKlare]

I see.

a. Total time of "the old way" > total time of "the new way" or (if not possible)
b. Total visits (from PropertyManager2) of "the old way" > total visits of "the new way"

Just to be sure: I would actually recommend neither of these options. Option "a" is a performance test, which I do not consider appropriate for the case that shall be tested here. Option "b" is still kind of a performance test and would not validate proper behavior but only validate somehow "better" behavior than some baseline (especially in terms of performance).
As mentioned before, I would recommend to convert this into a pure functionality that simply validates whether the new code behaves as expected (depth one traveral in specific situations).

[HeikoKlare]

I propose to merge this PR since the test is failing on MacOS very often. Still I would keep the issue open to have this performance test replaced with a functionality test as proposed in the comments. I will document / link to the according ideas posted in this PR in the issue when merging this PR.

[lathapatil]

performance test replaced with a functionality test as proposed in the comments.

Thanks for the input and I will be checking on replacing this with functionality test

[lathapatil]

I propose to merge this PR since the test is failing on MacOS very often. Still I would keep the issue open to have this performance test replaced with a functionality test as proposed in the comments. I will document / link to the according ideas posted in this PR in the issue when merging this PR.

@HeikoKlare The performance test is replaced with functional test. let me know if any further code changes are required .

[HeikoKlare]

Thank you for updating the test @lathapatil!
From a functional perspective, the test looks good. It serves as a regression test for #468 as intended.

I have some comments with respect to code style. Please also auto-format the code as it does currently not conform to the formatter settings. In addition, I would propose that you add the test case to the PropertyManagerTest class. Then the test case is placed next to other test cases concerning the same functionality. You can then also reuse existing functionality of that test class like a project setup for every test case and the WorkspaceTestRule that keeps track of properly cleaning up and makes custom teardown functionality obsolete.

[HeikoKlare]

Thank you for updating the test case. In general, it looks good now. I only have some minor comments remaining. Most important of them is the test method name, which should be more meaningful than just test().

[HeikoKlare]

Test case looks good now, thank you!

Failing tests do not seem to be related to this PR. I will check again soon.

[HeikoKlare]

Failing test is independent from this PR and documented in #128.