Add fixtures allowing e2e tests to be optionally skipped upon missing environment variables #720
Conversation
…e not available Signed-off-by: David Gardner <[email protected]>
dagardner-nv
added
improvement
|
Warning Rate limit exceeded@dagardner-nv has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a pytest plugin that gates integration/e2e tests on environment variables (new --fail_missing option), exposes fixtures for multiple API keys and services (MySQL, S3), provides a per-test environment restore fixture, decorates tests to use those fixtures, and removes a duplicate restore_environ from top-level conftest creating an unresolved dependency until fixed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as pytest CLI
participant Plugin as nat.test.plugin
participant Test as Integration Test
participant Helper as require_env_variables
participant OS as Environment
participant Service as External Service (MySQL/S3)
CLI->>Plugin: parse --fail_missing option
Note over Plugin: register fixtures (fail_missing, restore_environ, API key fixtures, service fixtures)
CLI->>Test: collect test (marked to use an API-key or service fixture)
Test->>Plugin: request fixture (e.g., `nvidia_api_key` or `mysql_server`)
alt API-key fixture requested
Plugin->>Helper: require_env_variables([VAR], reason, fail_missing)
Helper->>OS: read requested env vars
alt all vars present
OS-->>Helper: values
Helper-->>Plugin: dict of var->value
Plugin-->>Test: provide fixture value(s)
else missing vars and fail_missing = False
Helper-->>Test: call pytest.skip(reason)
else missing vars and fail_missing = True
Helper-->>Test: raise RuntimeError(reason)
end
else service fixture requested (MySQL/S3)
Plugin->>Service: attempt import/connect
alt service reachable and deps present
Service-->>Plugin: success
Plugin-->>Test: yield fixture
else missing deps or unreachable and fail_missing = False
Plugin-->>Test: call pytest.skip(reason)
else missing deps or unreachable and fail_missing = True
Plugin-->>Test: raise (propagate exception)
end
end
Plugin->>Test: (if provided) restore_environ context applied
Test->>Test: run test body
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/nvidia_nat_test/src/nat/test/plugin.py (8)
109-116: Fixture should return, not yield; also coerce to boolYield is unnecessary since there’s no teardown. Returning a real bool avoids accidental tri-state.
@pytest.fixture(scope="session") def fail_missing(pytestconfig: pytest.Config) -> bool: @@ - yield pytestconfig.getoption("fail_missing") + return bool(pytestconfig.getoption("fail_missing"))
136-145: Polish: brand casing, return type, and message consistency (OpenAI)
- Docstring: “OpenAI,” not “Openai.”
- Add an explicit return type for the fixture.
- Use consistent brand casing in the reason message.
-@pytest.fixture(name="openai_api_key", scope='session') -def openai_api_key_fixture(fail_missing: bool): +@pytest.fixture(name="openai_api_key", scope="session") +def openai_api_key_fixture(fail_missing: bool) -> dict[str, str]: """ - Use for integration tests that require an Openai API key. + Use for integration tests that require an OpenAI API key. """ yield require_env_variable( varnames=["OPENAI_API_KEY"], - reason="openai integration tests require the `OPENAI_API_KEY` environment variable to be defined.", + reason="OpenAI integration tests require the `OPENAI_API_KEY` environment variable to be defined.", fail_missing=fail_missing)
147-156: Polish: brand casing, return type (NVIDIA)Use “NVIDIA” consistently and add the fixture return type.
-@pytest.fixture(name="nvidia_api_key", scope='session') -def nvidia_api_key_fixture(fail_missing: bool): +@pytest.fixture(name="nvidia_api_key", scope="session") +def nvidia_api_key_fixture(fail_missing: bool) -> dict[str, str]: """ - Use for integration tests that require an Nvidia API key. + Use for integration tests that require an NVIDIA API key. """ yield require_env_variable( varnames=["NVIDIA_API_KEY"], - reason="Nvidia integration tests require the `NVIDIA_API_KEY` environment variable to be defined.", + reason="NVIDIA integration tests require the `NVIDIA_API_KEY` environment variable to be defined.", fail_missing=fail_missing)
158-170: Type hint for fixture return (AWS)Align fixture signature with actual return type for clarity.
-@pytest.fixture(name="aws_keys", scope='session') -def aws_keys_fixture(fail_missing: bool): +@pytest.fixture(name="aws_keys", scope="session") +def aws_keys_fixture(fail_missing: bool) -> dict[str, str]: @@ - yield require_env_variable( + yield require_env_variable( varnames=["AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY"], reason= "AWS integration tests require the `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY` environment variables to be " "defined.", fail_missing=fail_missing)
172-182: Grammar and return type (Azure OpenAI): “variables” pluralFix pluralization and add return type.
-@pytest.fixture(name="azure_openai_keys", scope='session') -def azure_openai_keys_fixture(fail_missing: bool): +@pytest.fixture(name="azure_openai_keys", scope="session") +def azure_openai_keys_fixture(fail_missing: bool) -> dict[str, str]: @@ - yield require_env_variable( + yield require_env_variable( varnames=["AZURE_OPENAI_API_KEY", "AZURE_OPENAI_ENDPOINT"], - reason="Azure integration tests require the `AZURE_OPENAI_API_KEY` and `AZURE_OPENAI_ENDPOINT` environment " - "variable to be defined.", + reason="Azure OpenAI integration tests require the `AZURE_OPENAI_API_KEY` and `AZURE_OPENAI_ENDPOINT` environment " + "variables to be defined.", fail_missing=fail_missing)
47-53: Option name LGTM; also consider small help-text/doc fixes in this block
--fail_missingis a useful toggle and integrates well with your fixtures.
- Nit: The help text for
--run_slowabove still says “Run end to end tests…”; consider updating it to “Run slow tests…”.- Nit: In the
pytest_addoptiondocstring (Line 23), “specfic” → “specific.”
16-17: Import placement and future-proof typingSince
require_env_variablenow usesSequence[str], add:from collections.abc import Sequencenear the existing imports. Grouping stdlib imports together keeps ruff happy.
118-134: Refactorrequire_env_variablefor robustness and typingThe existing use of
pytest.skip(reason=…)is valid—no change needed there. However, to improve robustness and adhere to typing guidelines, please apply the following optional refactors inpackages/nvidia_nat_test/src/nat/test/plugin.py:• Handle empty‐string values as missing:
Replace the currenttry/exceptonos.environ[var]with an explicit check for truthiness, e.g.:def require_env_variable(varnames: Sequence[str], reason: str, fail_missing: bool = False) -> dict[str, str]: """ Check that all required environment variables are set (non-empty) and return their values. If any are missing and `fail_missing` is False, skip the test; otherwise raise RuntimeError. """ - env_variables = {} - try: - for varname in varnames: - env_variables[varname] = os.environ[varname] - except KeyError as e: - if fail_missing: - raise RuntimeError(reason) from e - - pytest.skip(reason=reason) - - return env_variables + # Identify any vars that are missing or set to the empty string + missing = [v for v in varnames if not os.environ.get(v)] + if missing: + msg = f"{reason} Missing: {', '.join(missing)}." + if fail_missing: + raise RuntimeError(msg) + pytest.skip(msg=msg) + # All present and non-empty + return {v: os.environ[v] for v in varnames}• Use a more general input type:
At top of file, add:from collections.abc import SequenceAnd change the signature:
-def require_env_variable(varnames: list[str], reason: str, fail_missing: bool = False) -> dict[str, str]: +def require_env_variable(varnames: Sequence[str], reason: str, fail_missing: bool = False) -> dict[str, str]:These changes will ensure empty environment variables are treated as missing and align with our coding standards for parameter types.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/nvidia_nat_test/src/nat/test/plugin.py(3 hunks)tests/nat/llm_providers/test_langchain_agents.py(4 hunks)tests/nat/llm_providers/test_llama_index_agents.py(4 hunks)tests/nat/server/test_unified_api_server.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
{tests/**/*.py,examples/*/tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{tests/**/*.py,examples/*/tests/**/*.py}: Unit tests live in tests/ (or examples/*/tests) and use markers defined in pyproject.toml (e2e, integration)
Use pytest with pytest-asyncio for asynchronous tests
Mock external services with pytest_httpserver or unittest.mock; do not hit live endpoints in tests
Mark expensive tests with @pytest.mark.slow or @pytest.mark.integration to skip by default
Files:
tests/nat/llm_providers/test_langchain_agents.pytests/nat/server/test_unified_api_server.pytests/nat/llm_providers/test_llama_index_agents.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Format Python code with yapf (PEP8 base, column_limit=120) run second in the toolchain
Run ruff (ruff check --fix) using configuration in pyproject.toml; fix warnings unless explicitly ignored; ruff is a linter only (not for formatting)
Respect naming: snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Prefer httpx with SSL verification enabled by default for HTTP requests; follow OWASP Top-10
Use async/await for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial
Files:
tests/nat/llm_providers/test_langchain_agents.pytests/nat/server/test_unified_api_server.pytests/nat/llm_providers/test_llama_index_agents.pypackages/nvidia_nat_test/src/nat/test/plugin.py
**/*.{py,md,sh,yml,yaml,toml,json,ini,cfg,txt,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces (no tabs) and ensure every file ends with a single trailing newline
Files:
tests/nat/llm_providers/test_langchain_agents.pytests/nat/server/test_unified_api_server.pytests/nat/llm_providers/test_llama_index_agents.pypackages/nvidia_nat_test/src/nat/test/plugin.py
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}: Every file must start with the standard SPDX Apache-2.0 header
All source files must include the SPDX Apache-2.0 header template
Files:
tests/nat/llm_providers/test_langchain_agents.pytests/nat/server/test_unified_api_server.pytests/nat/llm_providers/test_llama_index_agents.pypackages/nvidia_nat_test/src/nat/test/plugin.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived by setuptools-scm; never hard-code versions in code or documentation
Files:
tests/nat/llm_providers/test_langchain_agents.pytests/nat/server/test_unified_api_server.pytests/nat/llm_providers/test_llama_index_agents.pypackages/nvidia_nat_test/src/nat/test/plugin.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: passDocumentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code should be licensed under the Apache License 2.0, and should contain an Apache License 2.0 header
comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
tests/nat/llm_providers/test_langchain_agents.pytests/nat/server/test_unified_api_server.pytests/nat/llm_providers/test_llama_index_agents.pypackages/nvidia_nat_test/src/nat/test/plugin.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/llm_providers/test_langchain_agents.pytests/nat/server/test_unified_api_server.pytests/nat/llm_providers/test_llama_index_agents.py
{src/**/*.py,packages/*/src/**/*.py,examples/*/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use abbreviations: 'nat' for API namespace and CLI tool, 'nvidia-nat' for package name, 'NAT' only for env var prefixes and informal code comments (never in documentation)
Files:
packages/nvidia_nat_test/src/nat/test/plugin.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code in packages must live under packages//src/
Files:
packages/nvidia_nat_test/src/nat/test/plugin.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{src/**/*.py,packages/*/src/**/*.py}: All public APIs require Python 3.11+ type hints on parameters and return values
Provide Google-style docstrings for every public module, class, function and CLI command
Docstring first line must be a concise description ending with a period
Files:
packages/nvidia_nat_test/src/nat/test/plugin.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_test/src/nat/test/plugin.py
🔇 Additional comments (3)
tests/nat/server/test_unified_api_server.py (1)
309-310: Gating e2e tests on NVIDIA_API_KEY via fixture is correctApplying
@pytest.mark.usefixtures("nvidia_api_key")to these endpoints aligns the e2e flow with the new env-gated plugin and keeps runs deterministic under--run_e2e. No further changes needed.Also applies to: 318-319, 327-328, 338-339, 353-354
tests/nat/llm_providers/test_langchain_agents.py (1)
30-31: Integration tests correctly gated by provider-specific fixturesUsing
usefixturesto require the appropriate credentials (NVIDIA/OpenAI/AWS/Azure) matches the plugin’s behavior and avoids spurious failures on partial-env setups.Also applies to: 54-55, 77-78, 105-106
tests/nat/llm_providers/test_llama_index_agents.py (1)
65-66: Consistent env gating for LlamaIndex provider testsThe added
usefixturesdecorators mirror the LangChain suite and ensure integration tests are skipped or failed uniformly depending on--fail_missing. Looks good.Also applies to: 78-79, 91-92, 112-113
Signed-off-by: David Gardner <[email protected]>
Signed-off-by: David Gardner <[email protected]>
…avid-e2e-testing Signed-off-by: David Gardner <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
packages/nvidia_nat_test/src/nat/test/plugin.py (8)
109-116: Use return (not yield) for simple, teardown-free fixture.No teardown is needed here; returning a bool is simpler and avoids generator-style fixtures.
@pytest.fixture(scope="session", name="fail_missing") def fail_missing_fixture(pytestconfig: pytest.Config) -> bool: """ Returns the value of the `fail_missing` flag, when false tests requiring unmet dependencies will be skipped, when True they will fail. """ - yield pytestconfig.getoption("fail_missing") + return bool(pytestconfig.getoption("fail_missing"))
118-134: Make env checks stricter and diagnostics clearer (collect all missing, treat empty as missing).Currently the helper stops at the first KeyError, does not flag empty-string values, and the message lacks which vars are missing. Collect them all and report clearly; this reduces confusion in CI.
-def require_env_variables(varnames: list[str], reason: str, fail_missing: bool = False) -> dict[str, str]: +def require_env_variables(varnames: Sequence[str], reason: str, fail_missing: bool = False) -> dict[str, str]: """ - Checks if the given environment variable is set, and returns its value if it is. If the variable is not set, and - `fail_missing` is False the test will ve skipped, otherwise a `RuntimeError` will be raised. + Checks if the given environment variables are set, and returns their values if they are. If any variable is not set, + and `fail_missing` is False the test will be skipped; otherwise a `RuntimeError` will be raised. """ - env_variables = {} - try: - for varname in varnames: - env_variables[varname] = os.environ[varname] - except KeyError as e: - if fail_missing: - raise RuntimeError(reason) from e - - pytest.skip(reason=reason) - - return env_variables + env_variables: dict[str, str] = {} + missing: list[str] = [] + for varname in varnames: + value = os.environ.get(varname) + if value is None or value.strip() == "": + missing.append(varname) + else: + env_variables[varname] = value + + if missing: + msg = f"{reason} Missing: {', '.join(missing)}." + if fail_missing: + raise RuntimeError(msg) + pytest.skip(reason=msg) + + return env_variablesFollow-up: This change updates the signature to use Sequence. Add the import shown in the next comment.
16-17: Import Sequence for the updated type annotation.import os +from collections.abc import Sequence
118-122: Fix typos in docstring.
- “ve skipped” → “be skipped”.
- Pluralize “variable(s)” when checking a list.
(Handled in the larger diff above; callout here so it’s not missed.)
136-145: Brand/style nits in OpenAI fixture.
- “Openai” → “OpenAI”
- Capitalize in the reason string for consistency.
@pytest.fixture(name="openai_api_key", scope='session') def openai_api_key_fixture(fail_missing: bool): """ - Use for integration tests that require an Openai API key. + Use for integration tests that require an OpenAI API key. """ yield require_env_variables( varnames=["OPENAI_API_KEY"], - reason="openai integration tests require the `OPENAI_API_KEY` environment variable to be defined.", + reason="OpenAI integration tests require the `OPENAI_API_KEY` environment variable to be defined.", fail_missing=fail_missing)
147-156: Brand/style nits in NVIDIA fixture.
- “Nvidia” → “NVIDIA” in both docstring and reason.
@pytest.fixture(name="nvidia_api_key", scope='session') def nvidia_api_key_fixture(fail_missing: bool): """ - Use for integration tests that require an Nvidia API key. + Use for integration tests that require an NVIDIA API key. """ yield require_env_variables( varnames=["NVIDIA_API_KEY"], - reason="Nvidia integration tests require the `NVIDIA_API_KEY` environment variable to be defined.", + reason="NVIDIA integration tests require the `NVIDIA_API_KEY` environment variable to be defined.", fail_missing=fail_missing)
172-181: Grammar nit: pluralize “variables”.The Azure reason string lists two env vars; pluralize “variables”.
yield require_env_variables( varnames=["AZURE_OPENAI_API_KEY", "AZURE_OPENAI_ENDPOINT"], - reason="Azure integration tests require the `AZURE_OPENAI_API_KEY` and `AZURE_OPENAI_ENDPOINT` environment " - "variable to be defined.", + reason="Azure integration tests require the `AZURE_OPENAI_API_KEY` and `AZURE_OPENAI_ENDPOINT` environment " + "variables to be defined.", fail_missing=fail_missing)
194-194: Add trailing newline.Ruff W292 flags the missing EOF newline.
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py (2)
20-20: Remove unused import.unittest.mock.patch isn’t used.
-import os -from unittest.mock import patch +import os
39-62: Optional: add coverage for empty/whitespace values if we choose to treat them as “missing.”If you adopt the stricter require_env_variables behavior (empty string → missing), extend the parameterization and tweak the has_missing computation accordingly.
Proposed changes:
- Add a case with an empty string value.
- Consider computing has_missing as: has_missing |= (value is None or (isinstance(value, str) and value.strip() == ""))
Example param update:
@pytest.mark.parametrize("env_vars", [ {"SOME_KEY": "xyz"}, {"SOME_KEY": "xyz", "OTHER_KEY": "abc"}, {"SOME_KEY": "xyz", "OTHER_KEY": "abc", "MISSING_KEY": None}, {"SOME_KEY": "xyz", "OTHER_KEY": "abc", "MISSING_KEY": None, "EMPTY_KEY": None}, {"SOME_KEY": "xyz", "OTHER_KEY": "", "ANOTHER_KEY": "abc"}, # empty string case ])
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/nvidia_nat_test/src/nat/test/plugin.py(3 hunks)packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py(1 hunks)tests/conftest.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/conftest.py
🧰 Additional context used
📓 Path-based instructions (9)
{src/**/*.py,packages/*/src/**/*.py,examples/*/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use abbreviations: 'nat' for API namespace and CLI tool, 'nvidia-nat' for package name, 'NAT' only for env var prefixes and informal code comments (never in documentation)
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.pypackages/nvidia_nat_test/src/nat/test/plugin.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code in packages must live under packages//src/
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.pypackages/nvidia_nat_test/src/nat/test/plugin.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Format Python code with yapf (PEP8 base, column_limit=120) run second in the toolchain
Run ruff (ruff check --fix) using configuration in pyproject.toml; fix warnings unless explicitly ignored; ruff is a linter only (not for formatting)
Respect naming: snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Prefer httpx with SSL verification enabled by default for HTTP requests; follow OWASP Top-10
Use async/await for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.pypackages/nvidia_nat_test/src/nat/test/plugin.py
**/*.{py,md,sh,yml,yaml,toml,json,ini,cfg,txt,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces (no tabs) and ensure every file ends with a single trailing newline
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.pypackages/nvidia_nat_test/src/nat/test/plugin.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{src/**/*.py,packages/*/src/**/*.py}: All public APIs require Python 3.11+ type hints on parameters and return values
Provide Google-style docstrings for every public module, class, function and CLI command
Docstring first line must be a concise description ending with a period
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.pypackages/nvidia_nat_test/src/nat/test/plugin.py
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}: Every file must start with the standard SPDX Apache-2.0 header
All source files must include the SPDX Apache-2.0 header template
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.pypackages/nvidia_nat_test/src/nat/test/plugin.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived by setuptools-scm; never hard-code versions in code or documentation
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.pypackages/nvidia_nat_test/src/nat/test/plugin.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: passDocumentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code should be licensed under the Apache License 2.0, and should contain an Apache License 2.0 header
comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.pypackages/nvidia_nat_test/src/nat/test/plugin.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.pypackages/nvidia_nat_test/src/nat/test/plugin.py
🧬 Code graph analysis (1)
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py (1)
packages/nvidia_nat_test/src/nat/test/plugin.py (1)
require_env_variables(118-133)
🪛 Ruff (0.12.2)
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py
20-20: unittest.mock.patch imported but unused
Remove unused import: unittest.mock.patch
(F401)
packages/nvidia_nat_test/src/nat/test/plugin.py
194-194: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (1)
packages/nvidia_nat_test/src/nat/test/plugin.py (1)
47-53: Good addition: explicit --fail_missing opt-in to fail instead of skip.This aligns well with mixed-cred CI use cases and keeps the default UX unchanged.
Signed-off-by: David Gardner <[email protected]>
…ted service is unavailable Signed-off-by: David Gardner <[email protected]>
Signed-off-by: David Gardner <[email protected]>
Signed-off-by: David Gardner <[email protected]>
…avid-e2e-testing Signed-off-by: David Gardner <[email protected]>
Signed-off-by: David Gardner <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py (2)
24-27: Avoid pinning Docker image versions in code commentsPer repo guidelines, don’t hard-code versions in .py files. Prefer “latest” or move the command to docs with guidance.
Apply within this block:
-# docker run --rm -ti --name test-mysql -e MYSQL_ROOT_PASSWORD=my-secret-pw -d -p 3306:3306 mysql:9.3 +# docker run --rm -ti --name test-mysql -e MYSQL_ROOT_PASSWORD=my-secret-pw -d -p 3306:3306 mysql:latest
50-58: Add return type to async context manager and consider env overrides for credentials
- Annotate the return type for _get_store for pyright correctness.
- Consider reading username/password from env with sensible defaults to align with the new gating philosophy.
Proposed changes within this block:
- async def _get_store(self): + async def _get_store(self) -> AsyncIterator[object]: async with WorkflowBuilder() as builder: await builder.add_object_store( "object_store_name", - MySQLObjectStoreClientConfig(bucket_name="test", username="root", password="my-secret-pw")) + MySQLObjectStoreClientConfig( + bucket_name="test", + username=os.getenv("NAT_TEST_MYSQL_USER", "root"), + password=os.getenv("NAT_TEST_MYSQL_PASSWORD", "my-secret-pw"), + ) + ) yield await builder.get_object_store_client("object_store_name")Additional import to add at the top:
import os from collections.abc import AsyncIteratorpackages/nvidia_nat_s3/tests/test_s3_object_store.py (2)
24-27: Avoid pinning Docker image versions in code commentsPer repo guidelines, don’t hard-code versions in .py files. Prefer “latest” or move to docs.
Apply within this block:
-# docker run --rm -ti -p 9000:9000 -p 9001:9001 minio/minio:RELEASE.2025-07-18T21-56-31Z \ +# docker run --rm -ti -p 9000:9000 -p 9001:9001 minio/minio:latest \ # server /data --console-address ":9001"
60-71: Type hints and env overrides for S3 configuration
- Add return type to _get_store for pyright.
- Allow endpoint and credentials to be overridden by env to align with gating elsewhere.
Apply within this block:
- async def _get_store(self): + async def _get_store(self) -> AsyncIterator[object]: async with WorkflowBuilder() as builder: await builder.add_object_store( "object_store_name", - S3ObjectStoreClientConfig(bucket_name="test", - endpoint_url="http://localhost:9000", - access_key="minioadmin", - secret_key="minioadmin")) + S3ObjectStoreClientConfig( + bucket_name="test", + endpoint_url=os.getenv("NAT_TEST_S3_ENDPOINT_URL", "http://localhost:9000"), + access_key=os.getenv("NAT_TEST_S3_ACCESS_KEY", "minioadmin"), + secret_key=os.getenv("NAT_TEST_S3_SECRET_KEY", "minioadmin"), + ) + ) yield await builder.get_object_store_client("object_store_name")Additional imports to add at the top:
import os from collections.abc import AsyncIterator
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py(1 hunks)packages/nvidia_nat_s3/tests/test_s3_object_store.py(1 hunks)packages/nvidia_nat_test/src/nat/test/plugin.py(3 hunks)packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py
- packages/nvidia_nat_test/src/nat/test/plugin.py
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Format Python code with yapf (PEP8 base, column_limit=120) run second in the toolchain
Run ruff (ruff check --fix) using configuration in pyproject.toml; fix warnings unless explicitly ignored; ruff is a linter only (not for formatting)
Respect naming: snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Prefer httpx with SSL verification enabled by default for HTTP requests; follow OWASP Top-10
Use async/await for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_mysql/tests/test_mysql_object_store.pypackages/nvidia_nat_s3/tests/test_s3_object_store.py
**/*.{py,md,sh,yml,yaml,toml,json,ini,cfg,txt,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces (no tabs) and ensure every file ends with a single trailing newline
Files:
packages/nvidia_nat_mysql/tests/test_mysql_object_store.pypackages/nvidia_nat_s3/tests/test_s3_object_store.py
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}: Every file must start with the standard SPDX Apache-2.0 header
All source files must include the SPDX Apache-2.0 header template
Files:
packages/nvidia_nat_mysql/tests/test_mysql_object_store.pypackages/nvidia_nat_s3/tests/test_s3_object_store.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived by setuptools-scm; never hard-code versions in code or documentation
Files:
packages/nvidia_nat_mysql/tests/test_mysql_object_store.pypackages/nvidia_nat_s3/tests/test_s3_object_store.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: passDocumentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code should be licensed under the Apache License 2.0, and should contain an Apache License 2.0 header
comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_mysql/tests/test_mysql_object_store.pypackages/nvidia_nat_s3/tests/test_s3_object_store.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_mysql/tests/test_mysql_object_store.pypackages/nvidia_nat_s3/tests/test_s3_object_store.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Pipeline / Check
🔇 Additional comments (2)
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py (1)
47-47: LGTM: Class-level gating via usefixturesThis ensures MySQL reachability is checked once per module.
packages/nvidia_nat_s3/tests/test_s3_object_store.py (1)
57-57: LGTM: Class-level gating via usefixturesThis matches the MySQL pattern and keeps service checks centralized.
willkill07
left a comment
willkill07
left a comment
There was a problem hiding this comment.
Approving with suggestion/comment
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py (1)
42-49: Deflake improvement correctly applied (remove missing vars from os.environ).Replacing the assertion with
os.environ.pop(..., None)avoids CI flakiness from pre-set envs. Good catch; this resolves the earlier review feedback.
🧹 Nitpick comments (2)
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py (2)
28-37: Clarify intent of "EMPTY_KEY": it represents an absent var, not an empty string.The name suggests an empty string value, but the test treats it as missing (None + pop). Rename to avoid confusion while keeping coverage for “multiple missing keys”.
- }, { - "SOME_KEY": "xyz", "OTHER_KEY": "abc", "MISSING_KEY": None, "EMPTY_KEY": None + }, { + "SOME_KEY": "xyz", "OTHER_KEY": "abc", "MISSING_KEY": None, "MISSING_KEY_2": None }])
19-24: Use collections.abc.Mapping for param type to align with guidelines.Prefer abstract types over concrete containers in annotations.
import os import pytest +from collections.abc import Mapping from nat.test.plugin import require_env_variables @@ -def test_require_env_variables(fail_on_missing: bool, env_vars: dict[str, str | None]): +def test_require_env_variables(fail_on_missing: bool, env_vars: Mapping[str, str | None]):Also applies to: 38-38
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/nvidia_nat_test/src/nat/test/plugin.py(3 hunks)packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nvidia_nat_test/src/nat/test/plugin.py
🧰 Additional context used
📓 Path-based instructions (9)
{src/**/*.py,packages/*/src/**/*.py,examples/*/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use abbreviations: 'nat' for API namespace and CLI tool, 'nvidia-nat' for package name, 'NAT' only for env var prefixes and informal code comments (never in documentation)
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code in packages must live under packages//src/
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Format Python code with yapf (PEP8 base, column_limit=120) run second in the toolchain
Run ruff (ruff check --fix) using configuration in pyproject.toml; fix warnings unless explicitly ignored; ruff is a linter only (not for formatting)
Respect naming: snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Prefer httpx with SSL verification enabled by default for HTTP requests; follow OWASP Top-10
Use async/await for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py
**/*.{py,md,sh,yml,yaml,toml,json,ini,cfg,txt,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces (no tabs) and ensure every file ends with a single trailing newline
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{src/**/*.py,packages/*/src/**/*.py}: All public APIs require Python 3.11+ type hints on parameters and return values
Provide Google-style docstrings for every public module, class, function and CLI command
Docstring first line must be a concise description ending with a period
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}: Every file must start with the standard SPDX Apache-2.0 header
All source files must include the SPDX Apache-2.0 header template
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived by setuptools-scm; never hard-code versions in code or documentation
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: passDocumentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code should be licensed under the Apache License 2.0, and should contain an Apache License 2.0 header
comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py
🧬 Code graph analysis (1)
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py (1)
packages/nvidia_nat_test/src/nat/test/plugin.py (1)
require_env_variables(118-133)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Pipeline / Check
🔇 Additional comments (3)
packages/nvidia_nat_test/src/nat/test/test_env_fixtures.py (3)
1-17: License header and module docstring look correct.SPDX headers are present and up-to-date, and the module has a concise docstring. Nothing to change here.
50-58: Exception/skip path validation is correct and targeted.Using
pytest.skip.Exceptionwithmatch="unittest"ensures the reason propagates. This matches the behavior of require_env_variables. No changes needed.
59-60: Success path equality check is precise.Asserting the returned dict equals the provided mapping (for non-missing cases) validates both presence and values. LGTM.
Signed-off-by: David Gardner <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py (1)
30-44: Close the MySQL connection, add a short connect timeout, and annotate the fixture returnThe fixture currently opens a connection and never closes it, and it can hang on unreachable hosts. Add a small timeout, ensure teardown closes the connection, clarify skip messages to mention --fail_missing, and add a precise return type.
Apply this diff within the fixture:
@pytest_asyncio.fixture(name="mysql_server", scope="module") -async def fixture_mysql_server(fail_missing: bool): +async def fixture_mysql_server(fail_missing: bool) -> AsyncIterator[None]: @@ - await aiomysql.connect(host='127.0.0.1', port=3306, user='root', password='my-secret-pw') - yield + conn = await aiomysql.connect( + host="127.0.0.1", + port=3306, + user="root", + password="my-secret-pw", + connect_timeout=2, + ) + try: + yield + finally: + conn.close() + await conn.wait_closed() @@ - pytest.skip("aiomysql not installed, skipping MySQL tests") + pytest.skip("aiomysql not installed; skipping MySQL tests (use --fail_missing to fail).") @@ - pytest.skip(f"Error connecting to MySQL server: {e}, skipping MySQL tests") + pytest.skip(f"Error connecting to MySQL server: {e}; skipping MySQL tests (use --fail_missing to fail).")Add the missing import at the top of the file:
from collections.abc import AsyncIterator
🧹 Nitpick comments (1)
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py (1)
51-59: Add a return type to _get_storeTests are Python files and our guidelines require type hints. Annotate the async contextmanager with an AsyncIterator return.
- async def _get_store(self): + async def _get_store(self) -> AsyncIterator[object]:Note: AsyncIterator is already proposed for import above. If you know the concrete client type, use it instead of object.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: Format Python code with yapf (PEP8 base, column_limit=120) run second in the toolchain
Run ruff (ruff check --fix) using configuration in pyproject.toml; fix warnings unless explicitly ignored; ruff is a linter only (not for formatting)
Respect naming: snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or extra metadata when useful
Treat pyright warnings (configured in pyproject.toml) as errors during development
Prefer httpx with SSL verification enabled by default for HTTP requests; follow OWASP Top-10
Use async/await for I/O-bound work (HTTP, DB, file reads)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial
Files:
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py
**/*.{py,md,sh,yml,yaml,toml,json,ini,cfg,txt,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Indent with 4 spaces (no tabs) and ensure every file ends with a single trailing newline
Files:
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{py,sh,md,yml,yaml,toml,json,ini,txt,cfg,rst,pyi}: Every file must start with the standard SPDX Apache-2.0 header
All source files must include the SPDX Apache-2.0 header template
Files:
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py
**/*.{py,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Version numbers are derived by setuptools-scm; never hard-code versions in code or documentation
Files:
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: passDocumentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code should be licensed under the Apache License 2.0, and should contain an Apache License 2.0 header
comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py
🧠 Learnings (1)
📚 Learning: 2025-08-25T15:01:54.259Z
Learnt from: CR
PR: NVIDIA/NeMo-Agent-Toolkit#0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-08-25T15:01:54.259Z
Learning: Applies to {tests/**/*.py,examples/*/tests/**/*.py} : Use pytest with pytest-asyncio for asynchronous tests
Applied to files:
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py
🔇 Additional comments (4)
packages/nvidia_nat_mysql/tests/test_mysql_object_store.py (4)
19-19: Good: switched to pytest-asyncio for async fixturesImport is correct and aligns with our guidance for async tests.
48-48: LGTM: class-level usefixtures ensures gating for all testsThis ensures the MySQL reachability check is applied uniformly to the suite.
25-27: No change needed: “mysql:9.3” tag is valid
We queried the Docker Hub API and confirmed that both9.3and8.4tags exist for the official MySQL image. Therefore, the recommendation to update the example to use a different tag is not needed.
- Confirmed available tags:
9.3,8.4- No updates required to the documentation snippet
Likely an incorrect or invalid review comment.
30-44: All fixtures and dependencies are wired correctly
- The
--fail_missingCLI option is registered inpackages/nvidia_nat_test/src/nat/test/plugin.pyviapytest_addoption(lines 21–29).- The
fail_missingfixture is defined in the same file (lines 109–113).aiomysqlis declared as a dependency inpackages/nvidia_nat_mysql/pyproject.toml(runtime dependency).pytest-asynciois listed under[tool.poetry.dev-dependencies]in the rootpyproject.toml.No changes are necessary; tests will collect and run as expected.
Signed-off-by: David Gardner <[email protected]>
|
/merge |
2 participants
Description
NVIDIA_API_KEYto runpytest --run_e2e --run_integrationand skip the tests requiring additional keys.--fail_missingflag which will prevent this and fail a test due to a missing key.By Submitting this PR I confirm:
Summary by CodeRabbit
Tests
Chores