Make AggregateActionxData More Amenable to Unit Testing#5239
Draft
bska wants to merge 3 commits into
Draft
Conversation
Member
Author
|
I'm creating this PR in draft mode because it depends on, and contains, the earlier PR #5238. I will keep the PR in a draft state until such time as it is ready for review. |
This commit revises the set of AggregateActionxData constructors to take simpler types. Previously, we needed a full Schedule object in order to form instances of AggregateActionxData and constructing Schedule objects, at least those that are viable for Actionx restart file unit testing, is very hard to do without parsing a mostly complete input file. This, in turn, made unit testing the class harder than necessary. The new canonical constructor takes the actual ActionX objects as a std::span<> and derives object and line counts from those. We still need a UnitSystem, well names, and start/elapsed times, but we now pass those through a new, dedicated "context" object instead of getting the values from a Schedule object. We also create two free function overloads to simplify transition to the new constructor, but expect that those will be removed in due time. For now though, they're used in the existing unit test for class AggregateActionxData.
abfe65b to
113666b
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Opm::RestartIO::Helpers::AggregateActionxData construction to remove the hard dependency on a fully-populated Schedule, making the type easier to instantiate in isolation for unit tests and other callers.
Changes:
- Introduces a new “canonical”
AggregateActionxDataconstructor that takesstd::span<const Action::ActionX>plus a dedicatedAggregateActionxRuntimeContext. - Adds
createAggregateActionxData(...)overloads to adapt existing call sites (notablySchedule-based usage) to the new constructor. - Updates restart-writing code and unit tests to use the new factory function and adds a new span-based constructor test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_AggregateActionxData.cpp | Adds a new standalone span-based constructor test and migrates existing tests to the factory helper. |
| opm/output/eclipse/RestartIO.cpp | Switches restart output generation to use createAggregateActionxData(...). |
| opm/output/eclipse/AggregateActionxData.hpp | Adds runtime context struct, span-based constructor, deprecated compatibility ctor, and factory function declarations. |
| opm/output/eclipse/AggregateActionxData.cpp | Implements span-based construction path and the new factory function overloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR revises the set of
AggregateActionxDataconstructors to take simpler types. Previously, we needed a fullScheduleobject in order to form instances ofAggregateActionxDataand constructingScheduleobjects, at least those that are viable for Actionx restart file unit testing, is very hard to do without parsing a mostly complete input file. This, in turn, made unit testing the class harder than necessary.The new canonical constructor takes the actual
ActionXobjects as astd::span<>and derives object and line counts from those. We still need aUnitSystem, well names, and start/elapsed times, but we now pass those through a new, dedicated "context" object instead of getting the values from aScheduleobject.We also create two free function overloads to simplify transition to the new constructor, but expect that those will be removed in due time. For now though, they're used in the existing unit test for class
AggregateActionxData.