Skip to content
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

[Bug]: [Medium Priority] Scripts under the test dir can no longer be run manually #1137

Closed
rickyloynd-microsoft opened this issue Jan 4, 2024 · 4 comments
Labels
0.2 Issues which are related to the pre 0.4 codebase

Comments

@rickyloynd-microsoft
Copy link
Contributor

Describe the bug

A change made by #1097 interferes with manual testing (not using pytest) of many scripts under the test dir because they now contain the following import:

from conftest import skip_openai

#1097 added test/conftest.py, but the test dir is not normally in sys.path, so manually running a script fails with this error:

ModuleNotFoundError: No module named 'conftest'

Maybe pytest avoids this error by appending the test dir to sys.path.

One solution is to add a line like the following right before the import of skip_openai:

sys.path.append(os.path.join(os.path.dirname(__file__), "../.."))

Similarly, 7 of our test scripts (like test_gpt_assistant.py) already append to sys.path in order to import from test_assistant_agent:

sys.path.append(os.path.join(os.path.dirname(__file__), ".."))
from test_assistant_agent import KEY_LOC, OAI_CONFIG_LIST # noqa: E402

I can make a PR modifying sys.path so that scripts under test can find conftest when run manually. Or is there a simpler solution?

Steps to reproduce

python test/agentchat/contrib/test_teachable_agent.py

Expected Behavior

The test should run to completion.

Screenshots and logs

No response

Additional Information

No response

@rickyloynd-microsoft
Copy link
Contributor Author

@maxim-saplin @sonichi Do either of you see a simpler solution?

@rickyloynd-microsoft
Copy link
Contributor Author

I just noticed that this is a dup of #1120. But I would give it a higher priority. We have blog posts directing users to run some of these tests.

@rickyloynd-microsoft rickyloynd-microsoft changed the title [Bug]: Scripts under the test dir can no longer be run manually [Bug]: [Medium Priority] Scripts under the test dir can no longer be run manually Jan 4, 2024
@maxim-saplin
Copy link
Contributor

@maxim-saplin @sonichi Do either of you see a simpler solution?

IMO, it's a trade-off between (1) keeping the code cleaner while loosing the ability to run tests via python test_xyz.py AND (2) going through every occasion of conftest import and preceding it with sys.path.append()

I can do the updates for (2) yet it seems (1) is the right way to avoid piling up code. -> @sonichi, @rickyloynd-microsoft your call

P.S.: to Run or Debug individual tests I am using VSCode's unit testing integration, either through left pane OR via the shortcut circle in the editor.

image

@rickyloynd-microsoft
Copy link
Contributor Author

it's a trade-off between (1) keeping the code cleaner while loosing the ability to run tests via python test_xyz.py AND (2) going through every occasion of conftest import and preceding it with sys.path.append()

Good summary. In my view, the cost of (1) is high, particularly for users when our docs invite them to run the test code directly, and the cost of (2) is low (just one line).

@rysweet rysweet added 0.2 Issues which are related to the pre 0.4 codebase needs-triage labels Oct 2, 2024
@rysweet rysweet closed this as not planned Won't fix, can't repro, duplicate, stale Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.2 Issues which are related to the pre 0.4 codebase
Projects
None yet
Development

No branches or pull requests

3 participants