|
| 1 | +:author: Gwendal Daniel |
| 2 | +:date: 2026-04-16 |
| 3 | +:status: accepted |
| 4 | +:consulted: Axel Richard |
| 5 | +:informed: Axel Richard, Pierre-Charles David |
| 6 | +:deciders: Axel Richard |
| 7 | +:pitch: |
| 8 | +:issue: https://github.com/eclipse-syson/syson/issues/2139 |
| 9 | + |
| 10 | += ADR-003 - Improve performances of integration tests |
| 11 | + |
| 12 | +== Context and Problem Statement |
| 13 | + |
| 14 | +SysON's build takes a lot of time to complete, especially on the CI. |
| 15 | +This creates uncomfortable situations for contributors, reviewers, and when releasing new versions of the application. |
| 16 | + |
| 17 | +Most of the execution time is spent building the backend, especially in running the integration tests. |
| 18 | +We identified a bottleneck in the way standard libraries are loaded and copied in `EditingContext` in each tests. |
| 19 | +These copy operations, in the context of the CI, eat up to 10 minutes of the build time (~30% of the backend build time). |
| 20 | + |
| 21 | +We want to reduce the time needed to run the integration tests, focusing first on this identified bottleneck. |
| 22 | + |
| 23 | +== Decision Drivers |
| 24 | + |
| 25 | +The existing implementation ensures that every `EditingContext` is loaded from scratch in each test. |
| 26 | +This limits the dependencies between tests by ensuring a corrupted `EditingContext` don't get reused in a later test. |
| 27 | +Altering this behavior one way or another, will add uncertainty regarding the context in which each test is executed. |
| 28 | + |
| 29 | +The first decision driver should be to agree with SysON contributors that the added uncertainty is fine and that we can manage its consequences (e.g. rewrite some tests, invest some time fixing the solution if an edge case is detected, etc). |
| 30 | + |
| 31 | +The selected solution should be as little invasive as possible: ideally, existing tests shouldn't be modified to benefit from it. |
| 32 | + |
| 33 | +Since the solution may create error-prone testing contexts (like shared resources), or involuntary dependencies between tests, it should be possible to de-activate it on a single test method, a test class, or the entire testing suite and run the tests as before. |
| 34 | + |
| 35 | +The solution should report testing context integrity issues it detects (e.g. shared resources that aren't available anymore). |
| 36 | + |
| 37 | +== Considered Options |
| 38 | + |
| 39 | +=== Cache standard libraries |
| 40 | + |
| 41 | +The `SysMLEditingContextProcessor` takes care of copying the standard libraries into a loading `EditingContext`. |
| 42 | +This happens in each test, since `EditingContext` are discarded as part of the `GivenInitialServerState#initialize` method (which ensures `EditingContext` are not shared between tests). |
| 43 | + |
| 44 | +We will replace this behavior in the tests to move the standard libraries from a shared cache instead of copying them. |
| 45 | +The first time an `EditingContext` will be loaded, its standard libraries will be copied as usual, and these libraries will be cached, allowing to retrieve them quickly the next time the `EditingContext` is loaded. |
| 46 | + |
| 47 | +We won't change the way `EditingContextProcessors` are disposed between tests, but we'll ensure that the standard libraries resources are removed from the `EditingContext` being discarded and put back into the cache to ensure they are reusable in latter tests (otherwise they'll be unloaded by `EditingContext#dispose`). |
| 48 | + |
| 49 | +We will provide methods and annotations to invalidate the cache, allowing to disable standard library caching on specific test methods and test classes. |
| 50 | +The caching mechanism will be activated only if the `org.eclipse.syson.test.cacheStandardLibraries` property is provided, otherwise, the default mechanism will be used and standard libraries will be copied every time they are needed. |
| 51 | + |
| 52 | + |
| 53 | +=== Use undo/redo to reset the editing context |
| 54 | + |
| 55 | +This solution relies on the undo capabilities provided by Sirius Web to reset the editing context into its initial state once a test has been completed. |
| 56 | + |
| 57 | +A new `IEditingContextDispatcher` will be provided to record inputs used by the tests. |
| 58 | +These inputs will be iterated backward once the test is completed to undo them in the right order. |
| 59 | + |
| 60 | +We will override `IGivenInitialServerState` to prevent the disposal of editing contexts, since they are now reset to their initial state. |
| 61 | + |
| 62 | +We will provide annotations to de-activate this behavior, and force the disposal of the `EditingContext` if needed. |
| 63 | + |
| 64 | +== Decision Outcome |
| 65 | + |
| 66 | +In the context of improving the execution time of integration tests, while keeping a relatively high confidence in the contexts in which tests are executed, we decided for the caching solution. |
| 67 | + |
| 68 | +The caching solution has the main advantage of being an in-house solution, which can be tweaked and adjusted as needed. |
| 69 | +For example, it can be configured to log inconsistent cache contents, informing the developer that something is probably wrong with a test. |
| 70 | + |
| 71 | +The undo solution relies on Sirius Web's ability to actually undo operations, and we don't have much control on this at the SysON level. |
| 72 | +Furthermore, there is currently no way to know if an undo operation was successfully performed or not. |
| 73 | +This solution also completely change the testing approach, by reusing the same `EditingContext` instance over and over, which seems too much to keep a good enough confidence level in the approach. |
| 74 | + |
| 75 | + |
| 76 | +== Consequences |
| 77 | + |
| 78 | +=== Advantages |
| 79 | + |
| 80 | +Tests that operate on the same `EditingContext` will run faster. |
| 81 | + |
| 82 | + |
| 83 | +=== Drawbacks |
| 84 | + |
| 85 | +The caching approach strongly relies on the assumption that only one client loads a given `EditingContext` at a time. |
| 86 | +Since standard libraries are moved and not copied, loading the same `EditingContext` two times will move the standard library resources from the first `EditingContext` to the second. |
| 87 | +This will typically lead to corrupted cached resources, which don't contain any element. |
| 88 | + |
| 89 | +Note that this issue appears when _loading_ the same `EditingContext` two times. |
| 90 | +It doesn't happen if an `EditingContext` is used multiple times in a test, for example by calling two runners, and opening two subscriptions. |
| 91 | +In these cases, the same `EditingContext` instance is reused, and the caching mechanism works as expected. |
| 92 | + |
| 93 | +From a contributor perspective, this limitation implies that `IEditingContextSearchService#findById` should never be called in tests. |
| 94 | +This method loads a new `EditingContext` instance, which will lead to caching issues. |
| 95 | +Instead, contributors can rely on `ExecuteEditingContextFunctionInput` and `IExecuteEditingContextFunctionRunner` to execute code inside an `EditingContext`. |
| 96 | + |
| 97 | +[NOTE] |
| 98 | +==== |
| 99 | +Using `IEditingContextSearchService#findById` in integration tests is considered a bad practice in Sirius Web. |
| 100 | +==== |
| 101 | + |
| 102 | +The initial implementation of this solution will require to change some calls to `IEditingContextSearchService#findById` to ensure the caches work properly. |
| 103 | + |
| 104 | + |
| 105 | +== More Information |
| 106 | + |
| 107 | + |
| 108 | +=== Implementation |
| 109 | + |
| 110 | +`AbstractIntegrationTests` will define an `@AfterEach` method that ensures standard libraries are removed from `EditingContext` instances before being disposed. |
| 111 | +Contributors should ensure their test calls `super.afterEach()` if they need to implement their own `@AfterEach` method. |
| 112 | + |
| 113 | +The `InvalidateStandardLibrariesCache` annotation can be used on test methods or test classes to invalidate standard libraries cache and ensure an `EditingContext` is loaded from scratch. |
| 114 | +Note that doing so will invalidate the cache for all the `EditingContexts`. |
| 115 | + |
| 116 | +The way standard libraries are removed from the `EditingContext` before its disposal could be implemented in an external service, as well as a dedicated `EditingContext` subclass, which would take care of filtering which resources to store in the cache before being disposed. |
| 117 | +While this solution would probably be cleaner (it wouldn't require an `@AfterEach` cleanup method), it would also add some distance between the code used to run the tests and the code used to run the application (different `EditingContext` implementation, most likely different `EditingContextSearchService` implementation, and maybe other related services). |
| 118 | +As a result, the external service approach will be used for an initial implementation. |
0 commit comments