-
Notifications
You must be signed in to change notification settings - Fork 35
feat: added pytest fixture to create and use its own temporary directory structure #363
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
base: main
Are you sure you want to change the base?
Conversation
…ory structure Signed-off-by: rohansen856 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great first step! 🥳
I'd love to see this being leveraged by other tests. In order to make this easier to maintain... we could move the creating of the zowe.config.json file in the tmp_path to the isolated_test_env function. Then we could create a class in conftest.py that other test classes can leverage...
Here is an example...
class TestIsolatedEnv(unittest.TestCase):
"""Base class for isolated test environments."""
@pytest.fixture(autouse=True)
def _setup_test_env(self, isolated_test_env):
self.isolated_test_env = isolated_test_envAnd then other tests can easily make use of the isolated_test_env by inheriting from it
class TestConsoleIntegration(TestIsolatedEnv):Also, for easier import, we may need to move conftest.py inside the integration folder (but that may cause other problems 😋
So we might be forced into creating a zowe.test_utils package for this class 😓
The above is just one way (of many) that we could implement this.
tests/conftest.py
Outdated
| project_dir = tmp_path / "project" | ||
| global_dir = tmp_path / "global" | ||
| project_dir.mkdir() | ||
| global_dir.mkdir() | ||
|
|
||
| return { | ||
| "project_dir": project_dir, | ||
| "global_dir": global_dir | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for keeping the globa vs. project configs here...
I believe that for the purpose of integration testing we could simplify this to just use the
tmp_path / zowe.config.json
| project_dir = tmp_path / "project" | |
| global_dir = tmp_path / "global" | |
| project_dir.mkdir() | |
| global_dir.mkdir() | |
| return { | |
| "project_dir": project_dir, | |
| "global_dir": global_dir | |
| } | |
| config_path = Path(__file__).resolve().parent.parent.parent / "zowe.config.json" | |
| config_file = tmp_path / "zowe.config.json" | |
| config_file.write_text(config_path.read_text()) | |
| return tmp_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zFernand0 @pem70 I think instead of tmp_path we need to return the config_file and config_path variables from the function. It would look something like this:
from pathlib import Path
import pytest
@pytest.fixture
def isolated_test_env(tmp_path):
config_path = Path(__file__).resolve().parent.parent.parent / "zowe.config.json"
config_file = tmp_path / "zowe.config.json"
config_file.write_text(config_path.read_text())
return {
"config_path": config_path,
"config_file": config_file
}And for the other file (tests/integration/test_integration_env.py):
def test_isolated_env_does_not_touch_real_home(isolated_test_env):
config_path = isolated_test_env["config_path"]
config_file = isolated_test_env["config_file"]
assert config_path.exists(), f"Missing config at {config_path}"
assert config_file.exists()
assert "profiles" in config_file.read_text()And yes this simplifies the overall structure by isolating all the logic and assertions in different functions🫡. Please let me know if I am mistaken anywhere...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is alright then we can possibly move towards making the structure more modular by using it like this: class TestIsolatedEnv(unittest.TestCase): as you suggested. A step by step approach would be better so I would like to first make a commit for modifications as mentioned above then move towards the modular integration...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zFernand0 is it ok to require zowe.config.json to be at the root level of the project, or should we allow it to reside at any folder level?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #363 +/- ##
=======================================
Coverage 88.05% 88.05%
=======================================
Files 65 65
Lines 3340 3340
=======================================
Hits 2941 2941
Misses 399 399
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hey @rohansen856, I'm sure there are better ways to avoid duplicating the creation/copy of the "project" zowe.config.json file into the tmp_path 😋 @t1m0thyj, @traeok, @pem70 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Same suggestions as @zFernand0 gave.
Thanks @zFernand0 😁... I tested the functions with the class TestIsolatedEnv(unittest.TestCase):
@pytest.fixture(autouse=True)
def isolated_test_env(tmp_path):
config_path = Path(__file__).resolve().parent.parent.parent / "zowe.config.json"
config_file = tmp_path / "zowe.config.json"
config_file.write_text(config_path.read_text())
return {
"config_path": config_path,
"config_file": config_file
}and the other file (test_integration_env.py) would be like this: from .conftest import TestIsolatedEnv
class TestIntegrationEnv(TestIsolatedEnv):
def test_isolated_env_does_not_touch_real_home(self, isolated_test_env):
config_path = isolated_test_env["config_path"]
config_file = isolated_test_env["config_file"]
self.assertTrue(config_path.exists(), f"Missing config at {config_path}")
self.assertTrue(config_file.exists())
self.assertTrue("profiles" in config_file.read_text())Here, instead of making the test integrations a separate module (which may complicate things unneccesarily), we can use the |
|
Hey @rohansen856, |
|
@zFernand0 I will start working on these from Wednesday at the earliest. I would like to work on this very PR as a draft (a lot more discussions might be ahead and we need to keep track of them in a single thread) and after all the things are finalized I can either request to merge this PR (if the overall structure is still clean at the end) or open up a fresh PR with all the changes in a systematic order... |
Signed-off-by: rohansen856 <[email protected]>
Signed-off-by: rohansen856 <[email protected]>
…s class Signed-off-by: rohansen856 <[email protected]>
|
@zFernand0 first of all sorry for the late! I have updated asll the integration test classes with the above discussed method (ex.: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Add pytest-based isolation for integration tests by introducing a temporary zowe.config.json fixture and updating existing tests to use it.
- Introduce
isolated_test_envfixture andTestIsolatedEnvbase class inconftest.py - Update existing integration test classes to inherit from
TestIsolatedEnvinstead ofunittest.TestCase - Add a new test (
test_integration_env.py) to verify the isolated environment setup
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/conftest.py | New pytest fixture and base class to set up an isolated test env |
| tests/integration/test_zosmf.py | Switched from unittest.TestCase to TestIsolatedEnv |
| tests/integration/test_zos_tso.py | Switched from unittest.TestCase to TestIsolatedEnv |
| tests/integration/test_zos_jobs.py | Switched from unittest.TestCase to TestIsolatedEnv |
| tests/integration/test_zos_files.py | Switched from unittest.TestCase to TestIsolatedEnv |
| tests/integration/test_zos_console.py | Switched from unittest.TestCase to TestIsolatedEnv |
| tests/integration/test_integration_env.py | New test verifying the isolated environment fixture |
Comments suppressed due to low confidence (3)
tests/integration/conftest.py:1
- The import of unittest isn’t used elsewhere; since pytest fixtures are in use, you can remove this import to simplify dependencies.
import unittest
tests/integration/conftest.py:3
- The imported TestCase symbol is not referenced in this file; you can remove it to avoid unused imports.
from unittest import TestCase
tests/integration/test_zosmf.py:3
- This import assumes
tests/integrationis a Python package; ensure there's an__init__.pysointegration.conftestcan be resolved.
from integration.conftest import TestIsolatedEnv
| "config_file": config_file | ||
| } | ||
|
|
||
| class TestIsolatedEnv(unittest.TestCase): |
Copilot
AI
Jul 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inheriting from unittest.TestCase while using pytest fixtures may lead to unexpected test collection behavior; consider defining TestIsolatedEnv as a plain class or using pytest’s xunit setup hooks instead.
| class TestIsolatedEnv(unittest.TestCase): | |
| class TestIsolatedEnv: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should consider this comment as we don't want this to negatively impact test collection behavior. Maybe we can pursue those xunit setup hooks.
| class TestIsolatedEnv(unittest.TestCase): | ||
| """Base class for isolated test environments.""" | ||
| @pytest.fixture(autouse=True) | ||
| def _setup_test_env(self, isolated_test_env): | ||
| self.isolated_test_env = isolated_test_env No newline at end of file |
Copilot
AI
Jul 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Autouse fixtures defined inside a TestCase subclass may not be applied as intended. Move this fixture to the module scope (top level in conftest.py) for pytest to recognize it properly.
| class TestIsolatedEnv(unittest.TestCase): | |
| """Base class for isolated test environments.""" | |
| @pytest.fixture(autouse=True) | |
| def _setup_test_env(self, isolated_test_env): | |
| self.isolated_test_env = isolated_test_env | |
| @pytest.fixture(autouse=True) | |
| def _setup_test_env(isolated_test_env): | |
| """Automatically set up the isolated test environment.""" | |
| return isolated_test_env | |
| class TestIsolatedEnv(unittest.TestCase): | |
| """Base class for isolated test environments.""" | |
| def setUp(self): | |
| self.isolated_test_env = _setup_test_env(isolated_test_env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this @rohansen856. I've left some comments - I believe we should also address the comments posted by Copilot.
| @@ -0,0 +1,9 @@ | |||
| """Test isolated environment setup for integration tests.""" | |||
|
|
|||
| def test_isolated_env_does_not_touch_real_home(isolated_test_env): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Does this actually verify that the environment does not touch the "real home"?
- When you mean "real home," are you referring to the user's home directory? or
ZOWE_CLI_HOME?
| "config_file": config_file | ||
| } | ||
|
|
||
| class TestIsolatedEnv(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should consider this comment as we don't want this to negatively impact test collection behavior. Maybe we can pursue those xunit setup hooks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this PR! 🙏
Just waiting for some of the comments to be address 🙏
What It Does
Fixes #361
Summary
As discussed in issue #361 I have added two files inside the tests directory which contains pytest fixtures to create and use a
tmp_dirfor the integration tests to run. This is the first part of incrementally implementing the directory isolation functionality for the integration tests.@zFernand0 If this fix is alright, For the next steps I would like to Implement
monkeypatchto temporarily change the Environment variables inside thetmp_diras to ascertain that the dev's local environment is not clashing with the tests.How to Test
Review Checklist
I certify that I have:
Additional Comments