Testing: Dynamic enablement/disablement of backends#225
Conversation
9b79060 to
69bb16f
Compare
There was a problem hiding this comment.
Pull request overview
This PR enables tests to dynamically enable/disable AMD backends at runtime by always instantiating all backends and making backend selection/IO consult Context<Configuration> at call time, rather than relying on one-time environment-variable parsing during initialization.
Changes:
- Refactors AMD
Configurationto support runtime override of fastpath/fallback enablement and makes env/proc-address checks lazy. - Updates backend instantiation/selection: always create Fastpath/Fallback; each backend now checks enablement in
score()/io()and can throwBackendDisabled. - Updates AMD unit/system tests and mocks to use the new configuration override approach; adjusts test CMake sources accordingly.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/system/amd/io.cpp | Switches system IO tests from env-var toggles to runtime Configuration enablement. |
| test/amd_detail/stats.cpp | Adds MConfiguration to control statsLevel() in StatsServer lifetime test. |
| test/amd_detail/mconfiguration.h | Introduces a Configuration mock with ContextOverride<Configuration>. |
| test/amd_detail/hipfile-api.cpp | Adds coverage for hipFileIo when backend io() throws BackendDisabled. |
| test/amd_detail/fastpath.cpp | Updates fastpath tests to expect config-gated scoring/IO via MConfiguration. |
| test/amd_detail/fallback.cpp | Updates fallback tests to expect config-gated scoring/IO via MConfiguration. |
| test/amd_detail/configuration.cpp | Reworks configuration tests around lazy env/proc checks + runtime overrides. |
| test/CMakeLists.txt | Moves system IO test source to AMD-only path. |
| src/amd_detail/static.h | Adds STATIC macro to disable function-local static caching under tests. |
| src/amd_detail/state.cpp | Always instantiates both backends rather than conditionally constructing them. |
| src/amd_detail/hip.cpp | Uses STATIC for cached proc-address function pointers. |
| src/amd_detail/configuration.h | Refactors configuration interface into overridable methods + override state. |
| src/amd_detail/configuration.cpp | Implements lazy env/proc checks and runtime override storage. |
| src/amd_detail/backend/fastpath.cpp | Gates scoring/IO on Configuration::fastpath(); throws BackendDisabled when disabled. |
| src/amd_detail/backend/fallback.cpp | Gates scoring/IO on Configuration::fallback(); throws BackendDisabled when disabled. |
| src/amd_detail/backend.h | Adds BackendDisabled exception type. |
Comments suppressed due to low confidence (1)
test/system/amd/io.cpp:90
HipFileIo::SetUp()mutates the globalContext<Configuration>enablement butTearDown()never restores it. Because system tests run in a single process, this can leak backend enablement state into later test cases and create order-dependent failures. Consider saving the previousfastpath()/fallback()values at the start ofSetUp()and restoring them inTearDown()(or explicitly re-enabling both backends after each test).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
If AIS_TESTING is defined, STATIC dissappears, if AIS_TESTING is not defined STATIC becomes static. This is useful in some testing scenarios.
…xt<Configuration> Previously, when MConfiguration inherited from IConfiguration compilers complained when you tried to do `StrictMock<MConfiguration>`. This will allow tests to use `StrictMock<MConfiguration> mcfg` and not worry about side effects of the `Configuration` constructor.
This will make it easier to add tests when fallback scoring is updated to test for fallback enablement.
Clone async tests to improve the name of test variants.
Before:
HipFileIo.ReadToUnregisteredBufferAtOffset/4-byte object <00-00 00-00>
HipFileIo.ReadToUnregisteredBufferAtOffset/4-byte object <01-00 00-00>
After:
HipFileIo.ReadToUnregisteredBufferAtOffset/Fastpath
HipFileIo.ReadToUnregisteredBufferAtOffset/Fallback
Backends now check for enablement when they score IO. If a backend is disabled it will not accept IO. Backends can be enabled/disabled at runtime. This was done for testing purposes. To fully support this functionality all backends need to be instantiated.
The new mechanism for selecting backends does not work when building for NVIDIA. The original mechanism for selecting backends did not work with NVIDIA either. Moving to an AMD specific directory to make it clear that these tests are AMD only (for now).
ad1564c to
a45a49e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
test/system/amd/io.cpp:70
SetUp()mutates the globalContext<Configuration>enablement (disables both backends, then enables one) butTearDown()doesn’t restore the previous configuration. Because system tests run in a single process, this can make later tests order-dependent (e.g., leaving fastpath disabled after the final parameterized case). Consider saving the prior fastpath/fallback values at the start ofSetUp()and restoring them inTearDown()(or otherwise resetting to a known default) to avoid cross-test contamination.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Motivation
Tests sometimes want to test a specific backend. Before this change backends were enabled by default and disabled using environment variables. Once the environment variables were parsed and the backends instantiated there was no way to modify which backends were enabled.
With this change all backends are always instantiated. Each backend checks its enablement when scoring and performing IO. Functions have been added to allow tests to dynamically change the enablement of each backend.
AIHIPFILE-157