[DSPX-3302] (4/5) xtest conftest: --scenario and --instance flags#453
[DSPX-3302] (4/5) xtest conftest: --scenario and --instance flags#453dmihalcik-virtru wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces new pytest CLI options, --scenario and --instance, and implements logic in pytest_configure to handle scenario-based defaults and environment variable propagation. The review feedback suggests using explicit is None checks for CLI options to ensure that user-provided empty strings are respected as overrides, rather than being ignored in favor of defaults. Additionally, a redundant import os statement was identified for removal.
| import os | ||
|
|
||
| instance = config.getoption("--instance") | ||
| if instance: | ||
| os.environ["OTDF_LOCAL_INSTANCE_NAME"] = instance |
There was a problem hiding this comment.
The import os statement is redundant as it is already imported at the top of the file (line 18). Additionally, the check if instance: should be replaced with if instance is not None: to correctly handle cases where an empty string is explicitly passed via the CLI, ensuring it is respected and not ignored in favor of scenario defaults.
| import os | |
| instance = config.getoption("--instance") | |
| if instance: | |
| os.environ["OTDF_LOCAL_INSTANCE_NAME"] = instance | |
| instance = config.getoption("--instance") | |
| if instance is not None: | |
| os.environ["OTDF_LOCAL_INSTANCE_NAME"] = instance |
References
- Redundant imports should be removed to maintain code cleanliness and avoid confusion.
- When implementing configuration overrides, ensure that explicit user input (even if empty) is prioritized over default values.
| if not config.getoption("--sdks-encrypt") and scenario.sdks.encrypt: | ||
| config.option.sdks_encrypt = " ".join(scenario.sdks.encrypt.keys()) | ||
| if not config.getoption("--sdks-decrypt") and scenario.sdks.decrypt: | ||
| config.option.sdks_decrypt = " ".join(scenario.sdks.decrypt.keys()) | ||
| if not config.getoption("--containers") and scenario.suite.containers: | ||
| config.option.containers = scenario.suite.containers | ||
| if not instance and scenario.instance.metadata.name: | ||
| os.environ["OTDF_LOCAL_INSTANCE_NAME"] = scenario.instance.metadata.name |
There was a problem hiding this comment.
Use is None checks for CLI options to ensure that explicit overrides (including empty strings) are respected and not overwritten by scenario defaults. This maintains the 'CLI wins' principle described in the function's docstring.
| if not config.getoption("--sdks-encrypt") and scenario.sdks.encrypt: | |
| config.option.sdks_encrypt = " ".join(scenario.sdks.encrypt.keys()) | |
| if not config.getoption("--sdks-decrypt") and scenario.sdks.decrypt: | |
| config.option.sdks_decrypt = " ".join(scenario.sdks.decrypt.keys()) | |
| if not config.getoption("--containers") and scenario.suite.containers: | |
| config.option.containers = scenario.suite.containers | |
| if not instance and scenario.instance.metadata.name: | |
| os.environ["OTDF_LOCAL_INSTANCE_NAME"] = scenario.instance.metadata.name | |
| if config.getoption("--sdks-encrypt") is None and scenario.sdks.encrypt: | |
| config.option.sdks_encrypt = " ".join(scenario.sdks.encrypt.keys()) | |
| if config.getoption("--sdks-decrypt") is None and scenario.sdks.decrypt: | |
| config.option.sdks_decrypt = " ".join(scenario.sdks.decrypt.keys()) | |
| if config.getoption("--containers") is None and scenario.suite.containers: | |
| config.option.containers = scenario.suite.containers | |
| if instance is None and scenario.instance.metadata.name: | |
| os.environ["OTDF_LOCAL_INSTANCE_NAME"] = scenario.instance.metadata.name |
References
- When implementing configuration overrides, ensure that explicit user input (even if empty) is prioritized over default values.
There was a problem hiding this comment.
Code Review
This pull request introduces new CLI options, --scenario and --instance, to the pytest configuration and implements a pytest_configure hook to manage scenario-based defaults and environment variable propagation. The review feedback highlights that using falsiness checks for CLI options prevents users from overriding scenario defaults with empty values; it is recommended to use is None checks instead to ensure CLI-passed options always take precedence. Additionally, a redundant local import of the os module was identified.
| import os | ||
|
|
||
| instance = config.getoption("--instance") | ||
| if instance: | ||
| os.environ["OTDF_LOCAL_INSTANCE_NAME"] = instance | ||
|
|
||
| scenario_path = config.getoption("--scenario") | ||
| if not scenario_path: | ||
| return | ||
| try: | ||
| from otdf_sdk_mgr.schema import load_scenario | ||
| except ImportError: | ||
| # otdf-sdk-mgr may not be installed in a minimal pytest env. | ||
| return | ||
| scenario = load_scenario(scenario_path) | ||
| if not config.getoption("--sdks-encrypt") and scenario.sdks.encrypt: | ||
| config.option.sdks_encrypt = " ".join(scenario.sdks.encrypt.keys()) | ||
| if not config.getoption("--sdks-decrypt") and scenario.sdks.decrypt: | ||
| config.option.sdks_decrypt = " ".join(scenario.sdks.decrypt.keys()) | ||
| if not config.getoption("--containers") and scenario.suite.containers: | ||
| config.option.containers = scenario.suite.containers | ||
| if not instance and scenario.instance.metadata.name: | ||
| os.environ["OTDF_LOCAL_INSTANCE_NAME"] = scenario.instance.metadata.name |
There was a problem hiding this comment.
The current implementation uses falsiness checks (if not ...) to determine if CLI options were provided. This prevents users from explicitly overriding scenario defaults with empty values (e.g., --sdks-encrypt "" to run no encryption SDKs), as an empty string is falsy and will be replaced by the scenario's default. Using is None checks ensures that only unprovided options are populated from the scenario file, adhering to the principle that CLI-passed options always win.
Additionally, the local import os is redundant as it is already imported at the top of the file (line 18).
instance = config.getoption("--instance")
if instance is not None:
os.environ["OTDF_LOCAL_INSTANCE_NAME"] = instance
scenario_path = config.getoption("--scenario")
if not scenario_path:
return
try:
from otdf_sdk_mgr.schema import load_scenario
except ImportError:
# otdf-sdk-mgr may not be installed in a minimal pytest env.
return
scenario = load_scenario(scenario_path)
if config.getoption("--sdks-encrypt") is None and scenario.sdks.encrypt:
config.option.sdks_encrypt = " ".join(scenario.sdks.encrypt.keys())
if config.getoption("--sdks-decrypt") is None and scenario.sdks.decrypt:
config.option.sdks_decrypt = " ".join(scenario.sdks.decrypt.keys())
if config.getoption("--containers") is None and scenario.suite.containers:
config.option.containers = scenario.suite.containers
if instance is None and scenario.instance.metadata.name:
os.environ["OTDF_LOCAL_INSTANCE_NAME"] = scenario.instance.metadata.nameThere was a problem hiding this comment.
Code Review
This pull request introduces new --scenario and --instance CLI options to the pytest configuration, along with a new pytest_configure function to handle scenario-based defaults and environment variable propagation. The review identifies a potential issue where the current logic for applying scenario defaults to SDK flags ignores the general --sdks CLI flag, and provides a code suggestion to ensure that CLI-provided options correctly take precedence.
| if not config.getoption("--sdks-encrypt") and scenario.sdks.encrypt: | ||
| config.option.sdks_encrypt = " ".join(scenario.sdks.encrypt.keys()) | ||
| if not config.getoption("--sdks-decrypt") and scenario.sdks.decrypt: | ||
| config.option.sdks_decrypt = " ".join(scenario.sdks.decrypt.keys()) |
There was a problem hiding this comment.
The current logic for applying scenario defaults to --sdks-encrypt and --sdks-decrypt does not account for the general --sdks CLI flag. Since pytest_generate_tests (lines 199-203 and 214-218) uses --sdks as a fallback when the specific flags are missing, programmatically setting config.option.sdks_encrypt or config.option.sdks_decrypt from the scenario file will cause any CLI-provided --sdks value to be ignored. To ensure that "CLI-passed options always win" as stated in the PR description, these defaults should only be applied if neither the specific flag nor the general --sdks flag was provided on the command line.
| if not config.getoption("--sdks-encrypt") and scenario.sdks.encrypt: | |
| config.option.sdks_encrypt = " ".join(scenario.sdks.encrypt.keys()) | |
| if not config.getoption("--sdks-decrypt") and scenario.sdks.decrypt: | |
| config.option.sdks_decrypt = " ".join(scenario.sdks.decrypt.keys()) | |
| if not config.getoption("--sdks-encrypt") and not config.getoption("--sdks") and scenario.sdks.encrypt: | |
| config.option.sdks_encrypt = " ".join(scenario.sdks.encrypt.keys()) | |
| if not config.getoption("--sdks-decrypt") and not config.getoption("--sdks") and scenario.sdks.decrypt: | |
| config.option.sdks_decrypt = " ".join(scenario.sdks.decrypt.keys()) |
There was a problem hiding this comment.
Code Review
This pull request enhances the pytest configuration by adding --scenario and --instance CLI options. A new pytest_configure hook is implemented to propagate the instance name via environment variables and to load default values for SDK and container settings from a scenario file when provided. The review feedback points out a redundant local import of the os module that should be removed to maintain code cleanliness and follow PEP 8 standards.
| via `OTDF_LOCAL_INSTANCE_NAME` so any child `otdf-local` invocation sees | ||
| the same instance. | ||
| """ | ||
| import os |
X-Test Results✅ go-v0.15.0 |
c69afd6 to
a8ef24a
Compare
6d2e83c to
f23ccce
Compare
a8ef24a to
78b2ca6
Compare
Adds two new pytest CLI options so xtest can be driven by a scenarios.yaml
and run against a specific otdf-local instance.
--scenario PATH When set, defaults --sdks-encrypt, --sdks-decrypt,
and --containers from the scenario's `sdks` and
`suite` blocks. Options explicitly passed on the
CLI always override.
--instance NAME Propagated to OTDF_LOCAL_INSTANCE_NAME so child
`otdf-local` invocations within the test see the
same instance the scenario expects.
If otdf-sdk-mgr is not installed (minimal pytest environments), the
--scenario flag silently no-ops via an ImportError guard. The flag
shape is invariant either way so CI configs don't fork.
This is the consumer side of the PR 3 / scenario-driven flow: the
authoritative entry point remains `otdf-local scenario run <path>`,
which sets these flags for you; this PR lets pytest accept them
directly when running scenario-aware sessions outside the wrapper.
Refs: https://virtru.atlassian.net/browse/DSPX-3302
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
f23ccce to
6498765
Compare
|



Summary
Fourth PR in the five-part stack. Adds two new pytest CLI options to
xtest/conftest.py:--scenario PATH— when set, defaults--sdks-encrypt,--sdks-decrypt, and--containersfrom the scenario file'ssdksandsuiteblocks. CLI-passed options always win.--instance NAME— propagated toOTDF_LOCAL_INSTANCE_NAMEso any nestedotdf-localinvocation agrees on the instance.The authoritative entry point remains
otdf-local scenario run <path>(PR 3), which sets these flags for you. This PR lets pytest accept them directly when running scenario-aware sessions outside the wrapper (e.g., a developer iterating withpytest --scenario foo.yaml -k some_test).The
--scenarioimport ofotdf_sdk_mgr.schemais guarded with a try/except so minimal pytest environments that don't haveotdf-sdk-mgrinstalled silently no-op.Stack
Test plan
cd xtest && uv run pytest --collect-only -q→ 103 tests still collect cleanlyuv run pytest --helpshows--scenarioand--instanceunder custom options--sdks-encrypt/--sdks-decrypt/--containersparametrizationJira: https://virtru.atlassian.net/browse/DSPX-3302
🤖 Generated with Claude Code