-
Notifications
You must be signed in to change notification settings - Fork 5
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
FI-2961 execute preset support #565
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #565 +/- ##
==========================================
+ Coverage 83.93% 83.94% +0.01%
==========================================
Files 263 263
Lines 11459 11460 +1
Branches 1261 1261
==========================================
+ Hits 9618 9620 +2
+ Misses 1831 1830 -1
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
lib/inferno/apps/cli/execute.rb
Outdated
[preset_input[:name], preset_input[:value]] | ||
end | ||
|
||
options.fetch(:inputs, {}).merge(preset_inputs) |
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 think this should go the other way, so that any explicitly defined inputs override the preset.
@@ -44,10 +44,12 @@ def run(options) | |||
|
|||
self.options = options | |||
|
|||
outputter.print_start_message(options) | |||
outputter.print_start_message(self.options) |
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.
This is fine, but you shouldn't need to say self.options
rather than just options
unless you are reassigning it since they both refer to the same object.
@@ -102,14 +104,28 @@ def outputter | |||
@outputter ||= OUTPUTTERS[options[:outputter]].new | |||
end | |||
|
|||
def load_preset_file_and_set_preset_id |
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 find this very confusing and am having a really hard time wrapping my head around this method. Like why does this method call apply_preset
, but that method isn't called anywhere if a preset id is used rather than a preset file?
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.
If the CLI is invoked with --preset-file
, this method will load the file into presets_repo
and reassign options[:preset_id]
to the id in the file. Aside from that its just bolting down some error cases.
I removed the test_session_repo#apply_preset
line which I forgot to do earlier. My understanding is test_session_repo#apply_preset
just adds the inputs to session_data_repo
, but persist_inputs
already does that in the CLI implementation (to both preset and direct inputs).
Summary
Adds two CLI options for
inferno execute
:--preset-id
that will load a preset by id from directories listed in lib/inferno/config/boot/presets.rb. This helps test kit authors.--preset-file
that will load a preset from a given file path. This helps test kit CI users.--preset-id
and--preset-file
cannot be used together. However--inputs
can be used with either of those options, and inputs will merge with and override presets.Testing Guidance
cp config/presets/dev_validator_preset.json tmp/my_preset.json
. Edit the file to change its id to something else unique.