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

tests in module java.refactoring are excessively slow #8160

Open
homberghp opened this issue Jan 16, 2025 · 7 comments
Open

tests in module java.refactoring are excessively slow #8160

homberghp opened this issue Jan 16, 2025 · 7 comments
Labels
kind:feature A feature request needs:triage Requires attention from one of the committers tests

Comments

@homberghp
Copy link
Contributor

homberghp commented Jan 16, 2025

Description

The first test in each test class takes 18.000 milliseconds on average.

With 31 test classes in the org.netbeans.modules.refactoring.java.test package alone, that is a huge waiting bill, or stated otherwise it takes my caffeine level over healthy values.
Since some of these tests involve the GUI, working on other things on the same machine is unproductive, because your work will lose focus.

Use case/motivation

Run the tests in package refactoring/netbeans/org.netbeans.modules.refactoring.java.test and measure.
NetBeans will do that for you. The last run on my machine (AMD64, 24 virtual cores, that do not help,32 Gigs of memory for me and netbeans alone).

Image

I would replace the method writeFilesAndWaitForScan with one that skips the wait for scan part. The wait for scan part is
documented as use with care, it takes a long time. From experience some, maybe a lot more can do without the full scan.

I will be waiting for approval before starting this effort.

Related issues

developing/adding features/resolve bugs. #7043, #7044

Are you willing to submit a pull request?

Yes

@homberghp homberghp added kind:feature A feature request needs:triage Requires attention from one of the committers labels Jan 16, 2025
@homberghp
Copy link
Contributor Author

I just found that some of the test methods are excessively long and have multiple unrelated asserts.

@homberghp
Copy link
Contributor Author

First tests reveal for CopyClassTest
with full scan:
Image

no scan:

Image

@homberghp
Copy link
Contributor Author

The brittleness of the tests appears to depend on the formatting of the expected code.
Reformatting to textblock and normalizing the layout (one stmt per line etc) improves both readability of actual and expected data and speeds up tests.

@homberghp
Copy link
Contributor Author

Does anyone know where I can find old issue numbers?
Numbers like 198186,187766 etc.

It's probably in some old tracking system. Is that still available? If so, it might be useful to add the issue description to the tests

@matthiasblaesing
Copy link
Contributor

Switching from writeFilesAndWaitForScan to an unsychronized version might be dangerous for test stability. We already see tests in CI, that more or less randomly fail. This becomes a problem when this happens to often, so this needs great care to check stability when these changes are done.

The old issue numbers might be from the old bugzilla instance, which should be archived here: https://bz.apache.org/netbeans/. They also might be from the Apache Jira instance https://issues.apache.org/jira/ (NETBEANS-.... is a good indicator, that these are from JIRA).

Refactorings outside bugfixing should have a very good reason. Review time is already limited, review without real benefit (yes I know that is discussable) gives little incentive.

@homberghp
Copy link
Contributor Author

I will investigate the stability.

Anyway, in the current setup, a failing test without writeFilesAndWaitForScan is rerun with writeFilesAndWaitForScan
before reporting failure.
The rerun mechanism was already set up and indicates unstable or flaky tests.

On the other hand: some of the flakiness appears to depend on badly formatted input and output.
Just by using text blocks and fixing up the broken input or expected test values, instead of long string chains with plusses to append them and test that fail on the timing because of a race between properly formatted code and improperly formatted code does not look very good to me either.

Maybe it's time someone invested some time in test stability. That should have a high priority. I and willing to put in my two cents.

@mbien mbien added the tests label Jan 23, 2025
@mbien
Copy link
Member

mbien commented Jan 25, 2025

performance and test reliability are two distinct issues

tests have often to wait for scanning to finish to get information about the changed code. Some might require the scan result itself, others might have to simply wait for the lucene index to finish writing before the workdir can be cleared for the next step.

removing waitScanFinished() without replacement won't work in most cases. I am sure things can be optimized, but it is likely more complicated and will also depend on the concrete test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature A feature request needs:triage Requires attention from one of the committers tests
Projects
None yet
Development

No branches or pull requests

3 participants