Skip to content

Added clarification for contributors who want to run tests #2736

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

andychan1998
Copy link

I tried following the instructions under the "Contribution" section, but ran into quite a few problems when trying to run the tests. It wasn't immediately obvious what the problem was since the test simply got stuck. Later, I realized that I needed to set OPENAI_API_KEY, SERPER_API_KEY, and CI in order to pass all the tests. I think adding this line would make it easier for future new contributors.

@joaomdmoura
Copy link
Collaborator

Disclaimer: This review was made by a crew of AI Agents.

Code Review Comment for PR #2736

Overview

This pull request introduces a valuable addition to the README.md file, offering clarification on environment variables required for running tests. Here's an analysis of the changes made and suggestions for further improvements.

Positive Aspects

  1. Increased Clarity: The additional text informs contributors about essential environment variables (OPENAI_API_KEY, SERPER_API_KEY) and introduces CI=1 for skipping tests related to local Ollama setup.
  2. Effective Use of Markdown: The addition aptly employs Markdown formatting, particularly the use of bold text to highlight key environment variables, enhancing readability.
  3. Clear Purpose: The explanation of CI=1 is straightforward, ensuring contributors understand its utility.

Suggestions for Improvement

  1. Environment Variable Section: Consider structuring the README to include a dedicated section for environment variables:

    ### Environment Variables for Testing
    Required:
    - **OPENAI_API_KEY**: Your OpenAI API key
    - **SERPER_API_KEY**: Your Serper API key
    
    Optional:
    - **CI=1**: Set this to skip tests that require a local Ollama setup.
  2. Local Testing Setup Instructions: To facilitate contributor onboarding, provide a step-by-step guide for local development setups:

    ### Local Testing Setup
    For local development:
    1. Copy `.env.example` to `.env`
    2. Add your API keys
    3. Choose test configuration:
        - Full test suite: Leave CI unset
        - Skip Ollama tests: Set CI=1
  3. Cross-Reference for Additional Resources: Including reference links can enrich the documentation:

    See [Contributing Guide](CONTRIBUTING.md) for more detailed testing instructions.

Related Pull Requests

Historically, enhancements made around the documentation of environment settings have greatly reduced onboarding friction for new contributors. Referencing past contributions can provide insights into effective communication strategies in documentation.

Impact Assessment

  • 🟢 Documentation Clarity: Enhanced
  • 🟢 Contributor Experience: Improved
  • 🟢 Test Setup Guide: More Comprehensive

Conclusion

Overall, the changes in this pull request are beneficial and significantly improve the clarity of the documentation. While the current addition is a step in the right direction, I recommend considering the suggested improvements in a follow-up PR to provide an even clearer and more structured guide for contributors. The changes can be merged as is, but incorporating these suggestions will further enhance documentation quality and user experience.

README.md Outdated
@@ -553,6 +553,8 @@ pre-commit install
uv run pytest .
```

In addition to **OPENAI_API_KEY** and **SERPER_API_KEY**, you may also want to set **CI=1** in your `.env` file to skip tests that require a local Ollama setup.
Copy link
Contributor

Choose a reason for hiding this comment

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

I really appreciate it when someone takes the time to suggest a documentation improvement 🙌

  1. why do you need to set SERPER_API_KEY? I’ve never had to add it before, so I’m curious what scenario you’re leading.
  2. Regarding CI=1, I'm planning to fix those Ollama tests in the coming days, so I think we can safely drop that part for now. Also, we're still unfortunately skipping some tests when CI is enabled (in which in not the ideal)
    I hope to address that soon, but I'd prefer not to encourage using it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify a few points here: I agree that to make all tasks pass, you need to set CI=1. However, I don’t think we officially recommend doing that

Copy link
Author

Choose a reason for hiding this comment

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

You are right, SERPER_API_KEY is not needed. I have updated to only mention OPENAI_API_KEY.

@andychan1998 andychan1998 requested a review from lucasgomide May 3, 2025 05:01
Copy link
Contributor

@lucasgomide lucasgomide left a comment

Choose a reason for hiding this comment

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

Thank you for your collaboration

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.

3 participants