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

Disentangle tests #1076 #1104

Merged
merged 8 commits into from
Apr 5, 2024
Merged

Disentangle tests #1076 #1104

merged 8 commits into from
Apr 5, 2024

Conversation

ATheorell
Copy link
Collaborator

As stated in #1076 the tests are way too tangled. In particular, usage of cached chatgpt responses in the tests have led to that most small changes have lead to multiple failing tests. This has been particularly challenging for new contributors.

In this PR:

  • No more caching in tests.
  • test_main.py no longer tests any workflow execution, only setup.
  • A single test in test_install.py runs the installed gpte executable WITH api key, no other test does. This test is marked with "requires_key" after suggestion by @ErikBjare. No other tests require api key.
  • Testing of running full improve workflows from the main currently missing, should be satisfied with specific unit tests of file_selector.py

@ATheorell
Copy link
Collaborator Author

Tox fails with exception that it cannot find valid OPENAI_API_KEY, though I've tried adding it in the ci.yaml

name: Run tox
env:
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
run: tox

There is a secret called OPENAI_API_KEY

Do you have an idea what could be going wrong here @ErikBjare ? I saw that you have a rather advanced setup including a key in https://github.com/ErikBjare/gptme/blob/master/.github/workflows/bot.yml

@ErikBjare
Copy link
Collaborator

ErikBjare commented Apr 4, 2024

@ATheorell I misunderstood, I'm not sure why it isn't working as it currently looks.

I figured it must be a quirk of tox, and I found this: https://tox.wiki/en/latest/config.html#passenv

So we need to set passenv=OPENAI_API_KEY (or whatever the syntax is), for tox to pass along the environment variable.

@ErikBjare
Copy link
Collaborator

ErikBjare commented Apr 5, 2024

@ATheorell Took the liberty, we'll see if it works...

Edit: looks like that did it!

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 47.36842% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 75.79%. Comparing base (1ad0892) to head (bdb4dab).

Files Patch % Lines
gpt_engineer/applications/cli/main.py 41.17% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1104      +/-   ##
==========================================
- Coverage   84.33%   75.79%   -8.54%     
==========================================
  Files          27       27              
  Lines        1564     1566       +2     
==========================================
- Hits         1319     1187     -132     
- Misses        245      379     +134     

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

@ATheorell ATheorell merged commit df593e1 into main Apr 5, 2024
6 checks passed
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.

2 participants