Skip to content

Conversation

@jonpspri
Copy link
Contributor

Summary

Fixes environment variable tests in test_translate_stdio_endpoint.py that were not properly verifying that environment variables were actually set in the subprocess.

Changes

  • Added _read_output() helper method to read subprocess output via pubsub subscription
  • Updated test script to accept environment variable names as command line arguments (instead of via stdin)
  • Rewrote 6 tests to read and verify actual environment variable values from subprocess output:
    • test_start_with_initial_env_vars
    • test_start_with_additional_env_vars
    • test_environment_variable_override
    • test_multiple_env_vars
    • test_env_vars_with_special_characters
    • test_large_env_vars
  • Removed unnecessary os.environ.copy() in test_multiple_env_vars

Test Plan

uv run pytest tests/unit/mcpgateway/test_translate_stdio_endpoint.py -v

All 21 tests pass successfully.

Impact

Tests now properly validate that environment variables are correctly passed to and set in the subprocess, rather than just checking that the process started.

Copilot AI review requested due to automatic review settings October 24, 2025 10:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes environment variable tests in test_translate_stdio_endpoint.py that were previously not verifying that environment variables were actually set in the subprocess. The tests now read and validate actual subprocess output instead of just checking process startup.

  • Added a _read_output() helper method to reliably read subprocess output via pubsub subscription
  • Modified the test script to accept environment variable names as command line arguments
  • Rewrote 6 tests to verify actual environment variable values from subprocess output

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@jonpspri jonpspri marked this pull request as draft October 24, 2025 12:43
@jonpspri jonpspri marked this pull request as ready for review October 24, 2025 21:03
@jonpspri jonpspri force-pushed the fix/test_translate_stdio_endpoint branch 2 times, most recently from 15b7fec to a0cfc84 Compare October 25, 2025 08:02
The tests in test_translate_stdio_endpoint.py were not properly
verifying environment variables. They sent variable names via stdin
instead of command line arguments, and never read subprocess output
to verify the values were actually set.

Changes:
- Add _read_output() helper to read subprocess output via pubsub
- Update test script to accept env var names as command line arguments
- Rewrite tests to read and verify actual environment variable values
- Remove unnecessary os.environ.copy() in test_multiple_env_vars

All tests now properly validate that environment variables are
correctly passed to and set in the subprocess.

Signed-off-by: Jonathan Springer <[email protected]>
Signed-off-by: Jonathan Springer <[email protected]>

Clean up async processing and RuntimeWarnings

Signed-off-by: Jonathan Springer <[email protected]>

Let's use jq where we can get away with it in tests

Signed-off-by: Jonathan Springer <[email protected]>

Fix a flake8 documentation find.

Signed-off-by: Jonathan Springer <[email protected]>

Pylint fixes

Signed-off-by: Jonathan Springer <[email protected]>
@jonpspri
Copy link
Contributor Author

@crivetimihai I inadvertently addressed the root cause of all those PR StdioIOEndpoint test failures we're seeing in the PR checks. LMK if I need to isolate it, otherwise, I'm lining up a bunch of PRs behind this one.

@crivetimihai crivetimihai merged commit 5ef6438 into IBM:main Oct 27, 2025
44 checks passed
@jonpspri jonpspri deleted the fix/test_translate_stdio_endpoint branch October 28, 2025 12:09
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.

2 participants