Skip to content

fix(tests): isolate test suite from global skillware config (#302) - #311

Open
tusharjamunkar wants to merge 1 commit into
ARPAHLS:mainfrom
tusharjamunkar:fix/isolate-pytest-global-config-302
Open

fix(tests): isolate test suite from global skillware config (#302)#311
tusharjamunkar wants to merge 1 commit into
ARPAHLS:mainfrom
tusharjamunkar:fix/isolate-pytest-global-config-302

Conversation

@tusharjamunkar

Copy link
Copy Markdown
Contributor

Description

Fixes #302.

Root Cause

When an operator initializes or configures Skillware locally (e.g., running skillware mail signature init), a global config.yaml is written to the user's config directory. When this file exists, get_skill_roots() switches to configured mode (project -> external -> bundled), causing tests that verify legacy resolution order (test_get_skill_roots_order_env_project_bundled and test_resolve_skill_prefers_env_over_cwd) to fail on developer machines.

Solution

  • Added an autouse=True fixture isolate_skillware_config in tests/conftest.py that points SKILLWARE_CONFIG_DIR (GLOBAL_CONFIG_DIR_ENV) to an isolated temporary directory for every test and clears the configuration cache before and after test execution.
  • Tests that explicitly mock or test custom config layers can continue to set GLOBAL_CONFIG_DIR_ENV or provide their own config files without conflict.

Verification

  • Verified with simulated global config.yaml: both test_get_skill_roots_order_env_project_bundled and test_resolve_skill_prefers_env_over_cwd pass.
  • Ran full test suite: 245 passed.
  • Lint check: flake8 tests/conftest.py passed cleanly.

)

Add an autouse fixture in tests/conftest.py to ensure tests run against an isolated temporary config directory and clean cache, preventing local/operator config.yaml from breaking legacy resolution order tests.
@rosspeili

Copy link
Copy Markdown
Contributor

Hey @tusharjamunkar thanks a lot for this, but as per contributing guide, and ai native workflow, please try to claim issues by commenting on them before opening a PR to avoid dublicates and excessive PR reviews.

Let me know if you'd like to pick another issue for now. Again, the easiest path woudl be to comment on the issue and claim it first. <3

@tusharjamunkar

Copy link
Copy Markdown
Contributor Author

Hi @rosspeili, thanks so much for the helpful heads-up and guidance! Understood completely — I will make sure to comment on issues to claim them first going forward as outlined in the contributing guidelines.

Since this fix is already implemented and verified with tests, please feel free to review/merge if convenient, or let me know if you'd prefer I close it and claim another issue first. Thanks again!

@tusharjamunkar

Copy link
Copy Markdown
Contributor Author

Also @rosspeili, if there is a specific open issue currently unclaimed / not being worked on that you recommend I tackle next, please feel free to point me towards it so I can comment and claim it. Happy to help!

@rosspeili

Copy link
Copy Markdown
Contributor

Hey @tusharjamunkar no worries at all, and appreciate the understanding. I would wait for the contributor who claimed #302 to open a PR, and if they stale or drop it, I will consider your PR, so do not close for now unless you wanna withdraw it. On top of that, there are a bunch of open issues, including good first issues. If you're specifically looking to work on tests, or config files, look for labels testing, and cli, or settings. Thanks, and welcome to Skillware <3

@tusharjamunkar

Copy link
Copy Markdown
Contributor Author

Thanks so much @rosspeili for the warm welcome and guidance! I'll leave this PR open as suggested.

Following your recommendation to check out testing issues, I'd love to take on #237 ([Feat]: CI smoke tests for local-execute examples). I will comment on that issue to claim it before getting started. Thanks again!

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.

[Bug]: Discovery tests fail when global config.yaml exists — isolate pytest from merged config

2 participants