Skip to content

fix: Rewrite environment tests to properly mock dependencies #19

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 20, 2025
Merged

fix: Rewrite environment tests to properly mock dependencies #19

merged 1 commit into from
May 20, 2025

Conversation

@zastrowm
Copy link
Member

@zastrowm zastrowm commented May 19, 2025 •
edited
Loading

Description

Rewrite environment tests to properly mock dependencies

  • Mock os.environ for test_environment by using a fixture, eliminating the need to worry about the real os environment.
  • Properly mock the get_user_input function by using a fixture as well and having environment import user_input as a module rather than importing the function directly. This is the more an important change as previously the user input wasn't being mocked in the tests - all tests were passing as the code paths didn't actually need "y".

As a result:

  • code-coverage of environment.py improved from 87% -> 92% as are now correctly testing the "y" flow where user input is needed
  • windows tests are now unblocked as get_user_input is now mocked and doesn't need an interactive terminal on windows (previously we got "No Windows console found. Are you running cmd.exe?" errors)

Related Issues

Interesting Notes

I initially struggled to get mocking of get_user_input to work properly. After debugging for a while - I noticed that the environment module being imported into the test differed from the one that the tool was executing within. I am unclear why, but this explains why mocking of get_user_input wasn't working (I tried several different approaches). Something to consider if we see odd behavior mocking going forward.

Type of Change

  • Bug fix

Testing

All of these

  • hatch fmt --linter
  • hatch fmt --formatter
  • hatch test --all

Checklist

  • I have read the CONTRIBUTING document
  • I have added tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • My changes generate no new warnings

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@zastrowm
fix: Rewrite environment tests to properly mock dependencies
1e410de 
Mock os.environ for test_environment by using a fixture, eliminating the need to worry about the real os environment.

Properly mock the get_user_input function by using a fixture as well and having environment import user_input as a module rather than importing the function directly.  This is the more an important change as previously the user input wasn't being mocked in the tests - all tests were passing as the code paths didn't actually need "y".
@zastrowm zastrowm marked this pull request as ready for review May 20, 2025 14:31
@zastrowm zastrowm requested a review from a team as a code owner May 20, 2025 14:31
@zastrowm zastrowm enabled auto-merge (squash) May 20, 2025 17:35
@zastrowm zastrowm merged commit f9b39fb into strands-agents:main May 20, 2025
33 checks passed
cagataycali pushed a commit to cagataycali/tools that referenced this pull request May 20, 2025
@zastrowm
fix: Rewrite environment tests to properly mock dependencies (strands…
116c2ae 
…-agents#19)

Mock os.environ for test_environment by using a fixture, eliminating the need to worry about the real os environment.

Properly mock the get_user_input function by using a fixture as well and having environment import user_input as a module rather than importing the function directly.  This is the more an important change as previously the user input wasn't being mocked in the tests - all tests were passing as the code paths didn't actually need "y".

Co-authored-by: Mackenzie Zastrow <[email protected]>
Unshure pushed a commit to Unshure/tools that referenced this pull request May 21, 2025
@zastrowm @Unshure
fix: Rewrite environment tests to properly mock dependencies (strands…
40faf02 
…-agents#19)

Mock os.environ for test_environment by using a fixture, eliminating the need to worry about the real os environment.

Properly mock the get_user_input function by using a fixture as well and having environment import user_input as a module rather than importing the function directly.  This is the more an important change as previously the user input wasn't being mocked in the tests - all tests were passing as the code paths didn't actually need "y".

Co-authored-by: Mackenzie Zastrow <[email protected]>
awsarron pushed a commit that referenced this pull request May 21, 2025
@Unshure @zastrowm @cagataycali
V0.1.2 (#24)
456042a 
* Add GitHub actions to run tests on Windows/MacOS & handle failures on Windows (#18)

Per #17 - Windows compatibility is currently unknown and all changes are currently only tested on Linux.  For [parity with the SDK](https://github.com/strands-agents/sdk-python/blob/912e1104fd52e3da99767a644b90b22b7eb606de/.github/workflows/test-lint-pr.yml#L59), start running tests on MacOS & Windows.

On MacOS all tests were already passing.  On Windows I:

 - added [`tzdata` as a dependency](https://pypi.org/project/tzdata/) to make the `current_time` work
 - lazy loaded `session: PromptSession` in `get_user_input` to avoid import errors as part of the tests 
 - updated [test_file_write.py](main...zastrowm:tools:windows_support#diff-a2998f18449d69766944a95d9058de448de22c38443a848876abaf22674df84a) to not use a permissions check, as the GitHub action runners seemingly run as super-admin and can write to the provided folders
 - disabled tests that were not trivial to fix - `test_shell`, `test_python_repl`
 - disabled some environment tests to better mock `get_user_input` - the two tests are failing on windows because of the lack of an interactive console.  I'd like to fix these two tests to properly mock `get_user_input` but I'm going to break that out into a separate PR to keep this one more focused.

To get linting to pass, I formatted the mem0 tool.

* fix: improve tool interoperability and remove dependencies (#14)

* chore: bump version from 0.1.0 to 0.1.1

* fix: improve tool interoperability and remove dependencies

This commit enhances multiple tool components to better work together:

- feat(think): inherit parent agent's traces and tools to maintain context
- fix(load_tool): remove unnecessary hot_reload_tools dependency check
- fix(use_llm): properly pass trace_attributes from parent agent to new instances
- style(mem0_memory): improve code formatting and readability
- test: update tests to match new implementation patterns

* fix(slack): add missing trace_attributes to Agent initialization

---------

Co-authored-by: Cagatay Cali <[email protected]>

* docs: update memory examples to use agent.tool.X syntax (#22)

* fix: Rewrite environment tests to properly mock dependencies (#19)

Mock os.environ for test_environment by using a fixture, eliminating the need to worry about the real os environment.

Properly mock the get_user_input function by using a fixture as well and having environment import user_input as a module rather than importing the function directly.  This is the more an important change as previously the user input wasn't being mocked in the tests - all tests were passing as the code paths didn't actually need "y".

Co-authored-by: Mackenzie Zastrow <[email protected]>

* Update version to v0.1.2 (#23)

---------

Co-authored-by: Mackenzie Zastrow <[email protected]>
Co-authored-by: ./c² <[email protected]>
Co-authored-by: Cagatay Cali <[email protected]>
Co-authored-by: Mackenzie Zastrow <[email protected]>
cagataycali added a commit that referenced this pull request Jun 9, 2025
@cagataycali @zastrowm
Feature: tool pass-through in use_llm and think tools (#72)
dca430b 
* chore: bump version from 0.1.0 to 0.1.1

* fix: improve tool interoperability and remove dependencies

This commit enhances multiple tool components to better work together:

- feat(think): inherit parent agent's traces and tools to maintain context
- fix(load_tool): remove unnecessary hot_reload_tools dependency check
- fix(use_llm): properly pass trace_attributes from parent agent to new instances
- style(mem0_memory): improve code formatting and readability
- test: update tests to match new implementation patterns

* fix(slack): add missing trace_attributes to Agent initialization

* docs(memory): update examples to use agent.tool.memory syntax

* fix: Rewrite environment tests to properly mock dependencies (#19)

Mock os.environ for test_environment by using a fixture, eliminating the need to worry about the real os environment.

Properly mock the get_user_input function by using a fixture as well and having environment import user_input as a module rather than importing the function directly.  This is the more an important change as previously the user input wasn't being mocked in the tests - all tests were passing as the code paths didn't actually need "y".

Co-authored-by: Mackenzie Zastrow <[email protected]>

* feat: add tool filtering to use_llm and think meta-tools

- Add 'tools' parameter to use_llm and think functions for parent tool filtering
- Implement tool registry filtering to prevent infinite recursion
- Allow parent agents to specify subset of tools for child agents
- Add comprehensive tests for tool filtering scenarios
- Update documentation with tool filtering examples
- Fix infinite recursion issue when use_llm calls use_llm or think calls think

This enhancement allows better control over meta-tool capabilities and prevents recursive loops while maintaining backward compatibility.

---------

Co-authored-by: Cagatay Cali <[email protected]>
Co-authored-by: Mackenzie Zastrow <[email protected]>
Co-authored-by: Mackenzie Zastrow <[email protected]>
Co-authored-by: Strands Agent <[email protected]>
jimbrub pushed a commit to jimbrub/tools that referenced this pull request Jun 23, 2025
@cagataycali @zastrowm @jimbrub
Feature: tool pass-through in use_llm and think tools (strands-agents#72
cbca77a 
)

* chore: bump version from 0.1.0 to 0.1.1

* fix: improve tool interoperability and remove dependencies

This commit enhances multiple tool components to better work together:

- feat(think): inherit parent agent's traces and tools to maintain context
- fix(load_tool): remove unnecessary hot_reload_tools dependency check
- fix(use_llm): properly pass trace_attributes from parent agent to new instances
- style(mem0_memory): improve code formatting and readability
- test: update tests to match new implementation patterns

* fix(slack): add missing trace_attributes to Agent initialization

* docs(memory): update examples to use agent.tool.memory syntax

* fix: Rewrite environment tests to properly mock dependencies (strands-agents#19)

Mock os.environ for test_environment by using a fixture, eliminating the need to worry about the real os environment.

Properly mock the get_user_input function by using a fixture as well and having environment import user_input as a module rather than importing the function directly.  This is the more an important change as previously the user input wasn't being mocked in the tests - all tests were passing as the code paths didn't actually need "y".

Co-authored-by: Mackenzie Zastrow <[email protected]>

* feat: add tool filtering to use_llm and think meta-tools

- Add 'tools' parameter to use_llm and think functions for parent tool filtering
- Implement tool registry filtering to prevent infinite recursion
- Allow parent agents to specify subset of tools for child agents
- Add comprehensive tests for tool filtering scenarios
- Update documentation with tool filtering examples
- Fix infinite recursion issue when use_llm calls use_llm or think calls think

This enhancement allows better control over meta-tool capabilities and prevents recursive loops while maintaining backward compatibility.

---------

Co-authored-by: Cagatay Cali <[email protected]>
Co-authored-by: Mackenzie Zastrow <[email protected]>
Co-authored-by: Mackenzie Zastrow <[email protected]>
Co-authored-by: Strands Agent <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Unshure Unshure Unshure approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@zastrowm @Unshure