-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add instructions to set up integration testing to contributor docs #135
Conversation
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to c97a066 in 12 seconds
More details
- Looked at
20
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. CONTRIBUTING.md:20
- Draft comment:
Consider specifying that these environment variables should be set in the shell or environment where the tests will be executed for clarity. - Reason this comment was not posted:
Confidence changes required:50%
The added instructions for setting environment variables are clear and necessary for running integration tests. However, it would be helpful to specify that these variables should be set in the shell or environment where the tests will be executed.
2. CONTRIBUTING.md:20
- Draft comment:
Ensure no secrets or credentials are hardcoded in the codebase. The environment variables for integration tests should be securely managed. - Reason this comment was not posted:
Confidence changes required:50%
The added environment variables for integration tests are a good addition to the documentation. However, the codebase should be checked for any hardcoded secrets or credentials.
Workflow ID: wflow_eg1g9EbZXiEJuzHi
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
thanks for contributing, @kylediaz . We're working to fix the int tests. |
It looks like the integration tests need these variables set in order to run properly. I didn't see it documented anywhere so I added it to the setup guide.
Also, is my project misconfigured or have the integration tests been broken for a a few weeks now? It looks like they have been broken since this commit, where you add new arguments to the function
invalidate_edges
, but don't update its uses intest_temporal_operations_int.py
.Important
Add environment variable setup instructions for integration tests to
CONTRIBUTING.md
.CONTRIBUTING.md
for setting environment variables required for integration tests:TEST_OPENAI_API_KEY
,TEST_OPENAI_MODEL
,NEO4J_URI
,NEO4J_USER
,NEO4J_PASSWORD
.This description was created by for c97a066. It will automatically update as commits are pushed.