-
Notifications
You must be signed in to change notification settings - Fork 31
[uss_qualifier/resources] Update naming and documentation for ResourceModifyingResource #1474
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
Open
BenjaminPelletier
wants to merge
3
commits into
interuss:main
Choose a base branch
from
BenjaminPelletier:adjust-resource-modifier
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| from .noop import NoOpResource as NoOpResource | ||
| from .test_exclusions import TestExclusionsResource as TestExclusionsResource | ||
| from .test_modifier import TestModifierModifierResource as TestModifierModifierResource | ||
| from .test_modifier import TestModifierResource as TestModifierResource | ||
| from .test_modifier import ( | ||
| NumberGeneratorModifierResource as NumberGeneratorModifierResource, | ||
| ) | ||
| from .test_modifier import NumberGeneratorResource as NumberGeneratorResource |
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
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
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
4 changes: 2 additions & 2 deletions
4
...er/TestModifierModifierSpecification.json → ...NumberGeneratorModifierSpecification.json
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
4 changes: 2 additions & 2 deletions
4
...t_modifier/TestModifierSpecification.json → ...odifier/NumberGeneratorSpecification.json
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be the opposite ? A different
keymust produce different variants; the same may produce equivalents result?I would assume that the spirit is to ensure different resources when needed because that more important that having the same values no?
(We can however have a 'must' for the same key -> same result)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the question; the change says "different key values -> different variants" + "same key -> equivalent results" -- the same thing as the original, just using "key" instead of "index".
One substantive difference is the removal of "(unique)" because that will probably often be the case, but there's no reason it necessarily needs to be in any case where we want a resource that spawns resources based on a template resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That indeed about the "(unique)" removed and the addition of 'generally': since the final goal is to be able to have resources that can be used in parallel without conflicts, I would expect that those resources generator, by 'contract' return different variants in every case, not in most of the cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case prompting the creation of this resource needs unique variants in all cases and should always produce equivalent results with equivalent keys. But the way this resource is defined allows it to be used in more general ways. Just because one use of a tool has certain requirements doesn't mean those requirements need to be imposed at the tool level rather than the tool-usage level. For instance, suppose we wanted to iterate over all ordered {uss1, uss2} pairs of participants, but a test needed a token that corresponded with the unordered pair of {uss1, uss2} (so {uss1, uss2} has a different token resource than {uss1, uss3}, but the same token resource as {uss2, uss1}). The suite may still be iterating over ordered pairs, but the resource producing modified versions of the underlying token resource may produce the same modified resource for multiple keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but should we then define an sub-type of the base type than enforce this? I would expect instances used in 'parallel generator' to follow stricter rules (This probably answer the type question bellow as well)
(True, but in that case since it's unordered, it would be an equal key ({uss1,uss2}=={uss2,uss1}) even if it's has different values. But that don't mean other cases wouldn't exists.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't expect we'd need to. A good test design will produce test scenarios that don't interfere with each other. Not interfering sometimes means "unique" modified resources (a necessary but not sufficient condition to deconflict geospatial resources), but not always (like in the case of iterating over ordered pairs but only needing unique unordered pairs for the token resource). But if we did need "unique" modified resources, I'd expect that to be introduced along with the thing that actually needs that constraint. The content in this PR (and #1465) doesn't need uniqueness -- the concept of stamping out modified copies of a template resource based on a key is a good standalone concept that doesn't need the additional constraint to work.
No, the key would be ordered as the test scenarios (or whatever is being primarily iterated) are distinguishable based on order in the example, but the particular resource being generated by modifying a base resource doesn't depend on order. To elaborate on the example: suppose we have a test scenario that has two roles and uses a token resource that needs to be "unique" according to participants in the scenario, but not according to what roles they're playing. The action generator would iterate over every (role1, role2) assignment of the {uss1, uss2, uss3, ...} participants when instantiating test scenarios. And, each test scenario would need a token resource based on some template token resource. But, the token resource needed would only depend on the participants rather than role assignments, so the modified token resources spawned would not be unique. At the same time, geospatial deconfliction is needed, so every set of flight intents would need to be "unique".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - that move the "responsibility" up to test definitions to only combine things that works together without an explicit 'contract', but that valid.