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

Let run tests directly via python test_xyz.py, fix for conftest import via sys.path.append #1144

Merged
merged 3 commits into from
Jan 5, 2024
Merged

Let run tests directly via python test_xyz.py, fix for conftest import via sys.path.append #1144

merged 3 commits into from
Jan 5, 2024

Conversation

maxim-saplin
Copy link
Collaborator

I have added sys.path.append() before all from conftest import skip_openai and also checked the test can be run directly.

Related issue number

Closes #1120 and #1137

Checks

@maxim-saplin maxim-saplin changed the title Let run tests directly via python test_xyz.py, fix for contest import via sys.path.append Let run tests directly via python test_xyz.py, fix for conftest import via sys.path.append Jan 4, 2024
Copy link
Contributor

@rickyloynd-microsoft rickyloynd-microsoft left a comment

Choose a reason for hiding this comment

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

Awesome!

@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (871e9e2) 30.74% compared to head (731cd55) 40.39%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1144      +/-   ##
==========================================
+ Coverage   30.74%   40.39%   +9.64%     
==========================================
  Files          30       30              
  Lines        4043     4043              
  Branches      915      964      +49     
==========================================
+ Hits         1243     1633     +390     
+ Misses       2721     2299     -422     
- Partials       79      111      +32     
Flag Coverage Δ
unittests 40.31% <ø> (+9.62%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@afourney afourney self-requested a review January 4, 2024 21:46
Copy link
Member

@afourney afourney left a comment

Choose a reason for hiding this comment

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

I think this is generally fine.

I think it's worth considering in the future how we maybe restructure the tests directory to maybe ameliorate this, and the need to add both the parent and grandparent paths.

Adding so many non-standard system paths feels a little suboptimal.

@maxim-saplin
Copy link
Collaborator Author

I think this is generally fine.

I think it's worth considering in the future how we maybe restructure the tests directory to maybe ameliorate this, and the need to add both the parent and grandparent paths.

Adding so many non-standard system paths feels a little suboptimal.

Totally agree and I don't like the smell of the PR :)

IMO it is better to drop the requirement to have the tests runnable by theirselves and revert to tooling (such as PyTest or IDE) - this will also allow to avoid adding main part at the bottom of files

@rickyloynd-microsoft
Copy link
Contributor

to avoid adding main part at the bottom of files

For me, the main part is a convenient way of selecting just the functions I want to run manually, without having to be in the IDE, or having to run pytest.

@sonichi sonichi added this pull request to the merge queue Jan 5, 2024
Merged via the queue into microsoft:main with commit 3f34365 Jan 5, 2024
76 of 84 checks passed
@afourney
Copy link
Member

afourney commented Jan 5, 2024

@rickyloynd-microsoft Yes, it's a pain (and not always ideal possible) to run every test in the file when trying to iterate on, or develop, one specific test (e.g. that you are adding for a new PR or something). Obviously we want to run ALL tests before committing, but it's not necessary when trying to step through or debug one particular one.

whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
…mport via `sys.path.append` (microsoft#1144)

* Runnig tests directly

* All tests with contrib add sys.path
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]: [Low Priority] Cannot run tests from a test's local folder. conftest is not found.
5 participants