Skip to content
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

Configuration #69

Merged
merged 14 commits into from
Dec 7, 2022
Merged

Configuration #69

merged 14 commits into from
Dec 7, 2022

Conversation

jhlee-mitre
Copy link
Contributor

@jhlee-mitre jhlee-mitre commented Nov 30, 2022

Add configuration

Overview

Ease of execution of the TestScript-engine requires specifying details of the execution as a part of the execution command and/or configuration file instead of via interactive prompts.

Requirements

  • All engine configuration settings must have documented default values that are used in the absence of explicit input.
  • Project-specific default values for all engine configuration settings must be specifiable within a configuration file.
  • All engine configuration settings must be specifiable at the command line as a part of the execution command, overriding the default or project-specific default values.
  • Users must be able to execute the engine in interactive mode where they can update the configuration settings as desired, but this should not be the default.

Proposed Approach

Add a YAML file to document the baseline configuration as below. The engine reads the file at the very beginning of execution.

# Endpoint against which TestScripts will be executed
server_url: http:https://server.fire.ly

# The relative path to the directory containing the TestScript resources (as JSON or XML) to be executed by the engine
testscript_path: "./TestScripts"
 
# Name(s) of TestScript under TESTSCRIPT_PATH to be executed
# If empty, all files under TESTSCRIPT_PATH will be executed
testscript_file: general_testscript.json

# The relative to the directory containing the TestReports output following their partner TestScript execution
testreport_path: "./TestReports"

# Whether to run the engine with interactive mode
interactive: false

# Whether to ignore non-FHIR fixtures
nonfhir_fixture: true
  
# Resource validator options:
# - "internal" uses the ruby validator from the fhir_models gem
#   https://github.com/fhir-crucible/fhir_models
# - "external" uses the a web interface for the L7 FHIR validator.
#   https://github.com/inferno-community/fhir-validator-wrapper
resource_validator: internal

# The url where the external validator can be reached. Ignored when using the internal validator
external_resource_validator_url: http:https://resource_validator_service:1234

# FHIRPath evaluator options: must be one of "internal" or "external".
fhirpath_evaluator: internal

# external_fhirpath_evaluator_url is only used if fhirpath_evaluator is set to external.
external_fhirpath_evaluator_url: http:https://fhirpath_validator_service:1234

User has an option to change the YAML based configuration when executing the engine using "parameters". All the configuration items in the YAML are supported with same naming convention. For example, server_url in YAML is same parameter name.

Test scenario for YAML - 1

In YAML, change server address:

server_url: localhost

Then run the engine without options

bundle exec bin/testscript_engine

Expected to see the server address changed.

Test scenario for YAML - 2

In YAML, interactive mode is off default. Change by:

interactive: true

Then run the engine without options

bundle exec bin/testscript_engine

Expected to run interactive mode.

Test scenario for Thor

In YAML, interactive mode is off default. Run the engine with option to run on interactive mode by:

bundle exec bin/testscript_engine option --interactive

Since Thor supersedes YAML, expected to run interactive mode.

@jhlee-mitre jhlee-mitre linked an issue Nov 30, 2022 that may be closed by this pull request
@jhlee-mitre jhlee-mitre marked this pull request as ready for review December 1, 2022 19:40
@jhlee-mitre jhlee-mitre self-assigned this Dec 1, 2022
@jhlee-mitre
Copy link
Contributor Author

jhlee-mitre commented Dec 1, 2022

Some issues:

  1. I synchronized parameter names in CLI and tag names in YAML for simplicity, but later parameter names in CLI need to follow CLI naming convention (a lot shorter)
  2. I didn't touch anything for existing interactive mode code. Later we will need to refactor.
  3. Class and module hierarchy of CLI and engine need to be reorganized.
  4. Unit test will be added.

@karlnaden
Copy link
Contributor

Good start

  1. Need some documentation on the config file and command line flags, with some examples, in the README file. Also would be nice to have a way to execute that outputs the options (e.g. exec ruby bin/testscript_engine option, which seems like it should do something, but it doesn't right now)
  2. I think there should be a command-line input to point to a different yaml file from which to load the config
  3. I think there should be a config option and command line flag to provide a list of runnable names to execute

@jhlee-mitre
Copy link
Contributor Author

Good start

  1. Need some documentation on the config file and command line flags, with some examples, in the README file. Also would be nice to have a way to execute that outputs the options (e.g. exec ruby bin/testscript_engine option, which seems like it should do something, but it doesn't right now)
  2. I think there should be a command-line input to point to a different yaml file from which to load the config
  3. I think there should be a config option and command line flag to provide a list of runnable names to execute

All good comments. I will keep moving on with them.

README.md Outdated Show resolved Hide resolved
@jhlee-mitre
Copy link
Contributor Author

All the suggestions were incorporated. Refer to the documentation below.

https://github.com/fhir-crucible/testscript-engine/blob/add_configuration/README.md#configure-the-engine

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@karlnaden karlnaden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

command line arguments failing for me right now:

knaden@MM272557-PC testscript-engine % bundle exec bin/testscript_engine --interactive
Could not find command "__interactive".
Deprecation warning: Thor exit with status 0 on errors. To keep this behavior, you must define `exit_on_failure?` in `TestScriptEngine::CLI::MyCLI`
You can silence deprecations warning by setting the environment variable THOR_SILENCE_DEPRECATION.
bundler: failed to load command: bin/testscript_engine (bin/testscript_engine)
/Users/knaden/Documents/Inferno/source/testscript-engine/lib/testscript_engine/cli.rb:38:in `start': undefined method `[]' for nil:NilClass (NoMethodError)
	from bin/testscript_engine:4:in `<top (required)>'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli/exec.rb:58:in `load'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli/exec.rb:58:in `kernel_load'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli/exec.rb:23:in `run'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli.rb:483:in `exec'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/vendor/thor/lib/thor.rb:392:in `dispatch'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli.rb:31:in `dispatch'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/vendor/thor/lib/thor/base.rb:485:in `start'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli.rb:25:in `start'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/exe/bundle:48:in `block in <top (required)>'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/friendly_errors.rb:103:in `with_friendly_errors'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/exe/bundle:36:in `<top (required)>'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/bin/bundle:23:in `load'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/bin/bundle:23:in `<main>'

I re-added the option key work as is in the examples and got further, but didn't see the server from the config file as the default as I expected and then ran into an error

knaden@MM272557-PC testscript-engine % bundle exec bin/testscript_engine option --interactive
Hello from the TestScriptEngine! The configuration is as follows: 

        SERVER UNDER TEST: []
        LOAD NON-FHIR FIXTURES: []
        TESTSCRIPT INPUT DIRECTORY or FILE: [/Users/knaden/Documents/Inferno/source/testscript-engine/TestScripts]
        TESTREPORT OUTPUT DIRECTORY: [/Users/knaden/Documents/Inferno/source/testscript-engine/TestReports] 

      Would you like to modify this configuration? [Y/N] N


   STARTING TO LOAD INPUT
bundler: failed to load command: bin/testscript_engine (bin/testscript_engine)
<internal:dir>:43:in `[]': no implicit conversion of nil into String (TypeError)
	from /Users/knaden/Documents/Inferno/source/testscript-engine/lib/testscript_engine.rb:58:in `load_input'
	from /Users/knaden/Documents/Inferno/source/testscript-engine/lib/testscript_engine/output/message_handler.rb:95:in `load_input'
	from /Users/knaden/Documents/Inferno/source/testscript-engine/lib/testscript_engine/cli.rb:72:in `start'
	from bin/testscript_engine:4:in `<top (required)>'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli/exec.rb:58:in `load'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli/exec.rb:58:in `kernel_load'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli/exec.rb:23:in `run'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli.rb:483:in `exec'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/vendor/thor/lib/thor.rb:392:in `dispatch'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli.rb:31:in `dispatch'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/vendor/thor/lib/thor/base.rb:485:in `start'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli.rb:25:in `start'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/exe/bundle:48:in `block in <top (required)>'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/friendly_errors.rb:103:in `with_friendly_errors'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/exe/bundle:36:in `<top (required)>'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/bin/bundle:23:in `load'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/bin/bundle:23:in `<main>'

Trying without interactive mode, the --runnable flag still doesn't seem to work:

bundle exec bin/testscript_engine option --runnable Gravityclinicalcaresdoh_Stu2_Sdohcccondition_Must_support_element_Condition_category.json
ERROR: "testscript_engine option" was called with arguments ["--runnable", "Gravityclinicalcaresdoh_Stu2_Sdohcccondition_Must_support_element_Condition_category.json"]
Usage: "testscript_engine option [OPTIONS]"
Deprecation warning: Thor exit with status 0 on errors. To keep this behavior, you must define `exit_on_failure?` in `TestScriptEngine::CLI::MyCLI`
You can silence deprecations warning by setting the environment variable THOR_SILENCE_DEPRECATION.
bundler: failed to load command: bin/testscript_engine (bin/testscript_engine)
/Users/knaden/Documents/Inferno/source/testscript-engine/lib/testscript_engine/cli.rb:38:in `start': undefined method `[]' for nil:NilClass (NoMethodError)
	from bin/testscript_engine:4:in `<top (required)>'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli/exec.rb:58:in `load'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli/exec.rb:58:in `kernel_load'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli/exec.rb:23:in `run'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli.rb:483:in `exec'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/vendor/thor/lib/thor.rb:392:in `dispatch'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli.rb:31:in `dispatch'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/vendor/thor/lib/thor/base.rb:485:in `start'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/cli.rb:25:in `start'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/exe/bundle:48:in `block in <top (required)>'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/lib/bundler/friendly_errors.rb:103:in `with_friendly_errors'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/lib/ruby/gems/3.0.0/gems/bundler-2.3.12/exe/bundle:36:in `<top (required)>'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/bin/bundle:23:in `load'
	from /Users/knaden/.asdf/installs/ruby/3.0.1/bin/bundle:23:in `<main>'

Several thoughts:

  1. The need for the work option within the command confuses me. Is there a reason it is needed?
  2. An expanded execution example of running with command line inputs would be helpful

@jhlee-mitre
Copy link
Contributor Author

Short answer for the "option" question: It's a method name in Thor.
See the example below.

class MyCLI < Thor
  desc "hello NAME", "say hello to NAME"
  def hello(name)
    puts "Hello #{name}"
  end
end

$ ruby ./cli hello Yehuda`

In my case, "option" is "hello".. It's hard to avoid the prefix. I believe Inferno has similar; console, migrate, etc.
Do you have an idea?

@karlnaden
Copy link
Contributor

I would choose something like execute for the command name instead of options, so you'd have bin/testscript_engine execute [options ...] runnable_name ...

@jhlee-mitre
Copy link
Contributor Author

I would choose something like execute for the command name instead of options, so you'd have bin/testscript_engine execute [options ...] runnable_name ...

Good idea. Let me take time to reorganize arguments..

@jhlee-mitre
Copy link
Contributor Author

I have two issues:

  1. Which is better to use runnable vs testscript_file in settings? Runnable sounds a bit developer's mind whereas testscript_file sounds straightforward, since it literally means file names of testscripts. I personally prefer testscript_file to minimize additional explanation in documents. Also aligned with naming convention with testscript_path. Let me know thoughts.
  2. I separated "interactive" and "config" from others. Rest of options are now under "execute". What does that look?

@karlnaden
Copy link
Contributor

karlnaden commented Dec 6, 2022

My thoughts on your questions:

  1. I would use testscript_name. You may want to load many files, but only execute 1 actual TestScript instance (e.g., if we add a composition approach). So this setting replaces runnable, but still selects the scripts based on the TestScript.name element.
  2. I personally wouldn't separate config and execute. I really think there should be a hierarchy of config
  • system defaults
  • config file (which may use the default config file or may be specified at the command line
  • execution-time specified options, either through command-line flags (or interactive, with this being a very low priority).

That said, I also think we shouldn't let getting this perfect get in the way of moving on to other features, so I'd recommend leaving them separated out for now and focus on making sure the functionality works as implemented.

Looks like you may be making more fixes, but things I've noticed when trying it

  • Need example of how to specify "runnables" in the config file. My initial attempts didn't work
  • execute didn't seem to be working:
knaden@MM272557-PC testscript-engine % bundle exec bin/testscript_engine execute --runnable Gravityclinicalcaresdoh_Stu2_Sdohcccondition_Must_support_element_Condition_category
Could not find command "execute".

@jhlee-mitre
Copy link
Contributor Author

My thoughts on your questions:

  1. I would use testscript_name. You may want to load many files, but only execute 1 actual TestScript instance (e.g., if we add a composition approach). So this setting replaces runnable, but still selects the scripts based on the TestScript.name element.

--> I like the idea. Thanks!

  1. I personally wouldn't separate config and execute. I really think there should be a hierarchy of config
  • system defaults
  • config file (which may use the default config file or may be specified at the command line
  • execution-time specified options, either through command-line flags (or interactive, with this being a very low priority).

That said, I also think we shouldn't let getting this perfect get in the way of moving on to other features, so I'd recommend leaving them separated out for now and focus on making sure the functionality works as implemented.

--> How do you assume to set "system default"? It was hard coded previously. I didn't like it and thought system default equals "config.xml". I believe you consider "config.xml" isn't system default, maybe that was the miscommunication.

Looks like you may be making more fixes, but things I've noticed when trying it

  • Need example of how to specify "runnables" in the config file. My initial attempts didn't work
  • execute didn't seem to be working:
knaden@MM272557-PC testscript-engine % bundle exec bin/testscript_engine execute --runnable Gravityclinicalcaresdoh_Stu2_Sdohcccondition_Must_support_element_Condition_category
Could not find command "execute".

--> Oh, I only committed README for review. I wanted to finalize code change once it shed the light. Sorry about confusion.

@jhlee-mitre
Copy link
Contributor Author

My thoughts on your questions:

  1. I would use testscript_name. You may want to load many files, but only execute 1 actual TestScript instance (e.g., if we add a composition approach). So this setting replaces runnable, but still selects the scripts based on the TestScript.name element.

--> I like the idea. Thanks!

FYI, TestScript.name is supposed to be computer friendly as opposed to TestScript.title is human readable. Our TestScripts examples are not like that. I don't think name like below can be used as argument/properties. Will need to reorganize TestScript examples.

"name": "TestScript Example History"

@karlnaden
Copy link
Contributor

karlnaden commented Dec 6, 2022

I personally wouldn't separate config and execute. I really think there should be a hierarchy of config
system defaults
config file (which may use the default config file or may be specified at the command line
execution-time specified options, either through command-line flags (or interactive, with this being a very low priority).
That said, I also think we shouldn't let getting this perfect get in the way of moving on to other features, so I'd recommend leaving them separated out for now and focus on making sure the functionality works as implemented.

--> How do you assume to set "system default"? It was hard coded previously. I didn't like it and thought system default equals "config.xml". I believe you consider "config.xml" isn't system default, maybe that was the miscommunication.

Yep, system defaults would be hard-coded. They won't be appropriate for everything, but for some:

  • server_url : no default? error if not specified?
  • nonfhir_fixture : not sure which
  • testscript_path : ./TestScript
  • testscript_file : no detault (all tests in path)
  • testreport_path : ./TestReport
  • ext_validator : none
  • ext_fhirpath : none
  • config_file: config.yaml

@jhlee-mitre
Copy link
Contributor Author

You can test with command below:

System configuration

bundle exec testscript_engine

Configuration file

bundle exec testscript_engine execute --config config.yml

CLI with a specific testscript name

bundle exec testscript_engine execute --testscript_name "TestScript Example General"

Interactive mode

bundle exec testscript_engine interactive

One remaining issue: It's slow to make multiple "selected" runnables execute. Somehow the current code set the variable runnable either nil or String. I don't fully understand yet how change of it to Array would impact. Do you mind if we save this an issue then come back later? I would hesitate to touch high level flow of the engine at this point.

@karlnaden
Copy link
Contributor

karlnaden commented Dec 6, 2022

Haven't done a close review of the code, but the execution patterns that are most important to me appear to work correctly:

  • execution based on default config file
  • execution based on specified config file with a specific testscript_name
  • execution with a command-line specified testscript_name

I think fine to defer multiple testscript_names until later. Recommend that this be merged in and continue with other features. We can iterate on this feature to improve it later as we get experience.

@jhlee-mitre
Copy link
Contributor Author

Haven't done a close review of the code, but the execution patterns that are most important to me appear to work correctly:

  • execution based on default config file
  • execution based on specified config file with a specific testscript_name
  • execution with a command-line specified testscript_name

I think fine to defer multiple testscript_names until later. Recommend that this be merged in and continue with other features. We can iterate on this feature to improve it later as we get experience.

Great, I will merge this. Will create a backlog item for multiple testscript names to come back later.

@jhlee-mitre jhlee-mitre merged commit 3a62c01 into main Dec 7, 2022
@jhlee-mitre jhlee-mitre deleted the add_configuration branch December 7, 2022 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration
2 participants