Skip to content

Conversation

@orthagh
Copy link
Contributor

@orthagh orthagh commented Dec 19, 2025

The idea was to generate a junit xml report to identify slow tests and try to optimize to top 5 slowest tests. Only 2 had optimization possible:

Summary:

1. OpenAPIGenerator Optimization 73831d5

locally 155s -> 4s (97%)

  • File: OpenAPIGenerator.php
  • Change: Implemented static caching for getComponentSchemas. This method was previously called multiple times during schema generation and resolution, leading to redundant calculations.
  • Impact: Reduced OpenAPIGeneratorTest execution time from 155s to 4s.

2. Optimization of testSearchAllMeta fe27e37

locally 111s -> 13s (88%)
The testSearchAllMeta test in tests/functional/SearchTest.php currently performs redundant searches by testing every search option of every meta-itemtype for every base itemtype. Since the JOIN logic between a base and a meta itemtype is independent of the specific search option chosen from the meta itemtype, we can significantly reduce the number of doSearch calls.

@orthagh orthagh force-pushed the task/long_tests_optimizations branch from 99c2556 to 1f2f702 Compare December 19, 2025 13:37
@orthagh orthagh force-pushed the task/long_tests_optimizations branch from 1f2f702 to 73831d5 Compare December 19, 2025 13:39
@orthagh orthagh marked this pull request as ready for review January 5, 2026 09:59
Copilot AI review requested due to automatic review settings January 5, 2026 09:59
@orthagh
Copy link
Contributor Author

orthagh commented Jan 5, 2026

Ok, I think I will no go further, I can't find more optimization that are really impactful.
OpenAPIGenerator part is straightfoward.
for testSearchAllMeta, I take your opinion @cedric-anne, it's a massive gain, but original purpose was to test everything. It now skip every joined searchoptions that are already has been tested once with another itemtype, and keep only the first SO for any following itemtypes.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR optimizes PHPUnit test performance by reducing execution time for two of the slowest tests through intelligent caching and redundancy elimination.

  • Implements static caching in OpenAPIGenerator to avoid redundant schema generation calls
  • Optimizes SearchTest to skip redundant meta-itemtype search option testing while maintaining coverage

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Glpi/Api/HL/OpenAPIGenerator.php Adds static caching to getComponentSchemas method, reducing execution time from 155s to 4s by avoiding redundant schema calculations
tests/functional/SearchTest.php Optimizes testSearchAllMeta by tracking fully-tested meta-itemtypes and skipping redundant search option tests, reducing execution time from 111s to 13s while maintaining test coverage

Comment on lines 144 to 147
static $cache = [];
if (isset($cache[$api_version])) {
return $cache[$api_version];
}
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

The static cache in getComponentSchemas may return stale data if the list of controllers changes between calls (e.g., when plugins are enabled/disabled during tests). The cache is keyed only by api_version but the method depends on Router::getInstance()->getControllers() which can vary based on the application state. Consider adding the controllers state to the cache key or implementing a cache invalidation mechanism when the router state changes.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

The bot is technically correct (the best kind of correct) and I think it is why there isn't much caching currently here. There are other cases that may fail like if a new custom asset definition is added during the tests, or if permissions change, and therefore the available schemas change.

I think the preferred way to handle per-request caching is a private property on the class and a method to allow clearing it as needed on a test-by-test basis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Var has been moved to a private property, and a method to clean cache added.
I have no clue if the method is already needed in the tests' suite that said

@cconard96
Copy link
Contributor

Ok, I think I will no go further, I can't find more optimization that are really impactful. OpenAPIGenerator part is straightfoward. for testSearchAllMeta, I take your opinion @cedric-anne, it's a massive gain, but original purpose was to test everything. It now skip every joined searchoptions that are already has been tested once with another itemtype, and keep only the first SO for any following itemtypes.

I'm not sure it can be 100% certain that the joins will not change for a meta itemtype based on the searched itemtype currently or in the future.
There are a lot of special cases in Glpi/Search/Provider/SQLProvider::getMetaLeftJoinCriteria.

@cedric-anne
Copy link
Member

Ok, I think I will no go further, I can't find more optimization that are really impactful. OpenAPIGenerator part is straightfoward. for testSearchAllMeta, I take your opinion @cedric-anne, it's a massive gain, but original purpose was to test everything. It now skip every joined searchoptions that are already has been tested once with another itemtype, and keep only the first SO for any following itemtypes.

I'm not sure it can be 100% certain that the joins will not change for a meta itemtype based on the searched itemtype currently or in the future. There are a lot of special cases in Glpi/Search/Provider/SQLProvider::getMetaLeftJoinCriteria.

When I introduced this test, the main issues here were:

  1. lack of alias in joined table, and this will still be tested with a search on the first criterion;
  2. conflict on aliases, and this is done later in the test.

Therefore, I think the proposed change is OK.

orthagh and others added 2 commits January 7, 2026 16:08
Co-authored-by: Johan Cwiklinski <trasher@x-tnd.be>
@trasher trasher merged commit 2186bc6 into glpi-project:11.0/bugfixes Jan 8, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants