Feature/revive integration tests #1343
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 9d0c824 in 24 seconds
More details
- Looked at
176lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_eKKYehSlG96uogkx
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| sudo systemctl start postgresql.service | ||
| sudo -u postgres psql -c "ALTER USER postgres PASSWORD 'postgres';" | ||
|
|
||
| # Add your other steps here |
There was a problem hiding this comment.
The workflow is missing steps to set up Python, install dependencies, and run the integration tests. These steps were present in the original workflow and are necessary for the integration tests to run.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 32bba85 in 23 seconds
More details
- Looked at
163lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/integration-test-workflow-debian.yml:25
- Draft comment:
The workflow is missing a step to install and configure PostgreSQL. Ensure PostgreSQL is installed and running before starting the R2R server. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_5OnMlLFKACZ6PkpC
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
py/tests/integration/runner.py
Outdated
|
|
||
| def run_command(command): | ||
| result = subprocess.run( | ||
| command, shell=True, capture_output=True, text=True |
There was a problem hiding this comment.
Using shell=True can be a security risk if the command includes unsanitized input. Consider using a list of arguments instead, e.g., subprocess.run(['poetry', 'run', 'r2r', 'ingest-sample-files'], capture_output=True, text=True).
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 04c437c in 11 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/integration-test-workflow-debian.yml:43
- Draft comment:
Consider using a loop to check if the R2R server is ready instead of a fixed sleep time. This can prevent unnecessary delays or race conditions. - Reason this comment was not posted:
Confidence changes required:50%
The current implementation uses a fixed sleep time to wait for the R2R server to start. This can lead to unnecessary delays or race conditions if the server takes longer to start.
Workflow ID: wflow_sdEk310BLMq6nEi6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 6843c4b in 17 seconds
More details
- Looked at
23lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_mLazMvPY6YJ4S7KN
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| POSTGRES_HOST: ${{ secrets.POSTGRES_HOST }} | ||
| POSTGRES_PORT: ${{ secrets.POSTGRES_PORT }} | ||
| R2R_PROJECT_NAME: ${{ secrets.R2R_PROJECT_NAME }} | ||
| POSTGRES_USER: postgres |
There was a problem hiding this comment.
Using hardcoded PostgreSQL credentials is not secure. Consider using GitHub secrets for these values to enhance security.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5bebd3b in 8 seconds
More details
- Looked at
29lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/integration-test-workflow-debian.yml:28
- Draft comment:
Consider enabling PostgreSQL to start on boot for better reliability.
sudo systemctl enable postgresql.service
- Reason this comment was not posted:
Confidence changes required:50%
The installation of PostgreSQL should ensure that the service is enabled to start on boot, which is a common best practice.
Workflow ID: wflow_d1u0NuXeULm0FvJ0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2924dfa in 19 seconds
More details
- Looked at
170lines of code in4files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. py/tests/conftest.py:75
- Draft comment:
Consider using theapp_configfixture directly incrypto_providerinstead of passing it as an argument, as it is already available in the session scope. This applies to other fixtures likepostgres_db_provider,temporary_postgres_db_provider, andr2r_auth_provideras well. - Reason this comment was not posted:
Confidence changes required:50%
Theapp_configfixture is defined but not used in some places where it could be beneficial for consistency and potential future use.
2. .github/workflows/integration-test-workflow-debian.yml:23
- Draft comment:
Usingpkill -f "r2r serve"can terminate unintended processes if multiple instances are running. Consider using a more specific method to stop the server, such as storing the process ID and killing that specific process. - Reason this comment was not posted:
Confidence changes required:50%
Thepkillcommand in the GitHub Actions workflow might terminate unintended processes if multiple instances ofr2r serveare running. It's safer to use a more specific method to stop the server.
Workflow ID: wflow_yKvjZNQcuALQjYKL
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b11ce36 in 9 seconds
More details
- Looked at
126lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/integration-test-workflow-debian.yml:36
- Draft comment:
Consider checking out a specific commit or tag for pgvector to ensure consistent builds. - Reason this comment was not posted:
Confidence changes required:50%
The installation of pgvector in the GitHub Actions workflow is not using a specific version or commit hash. This can lead to potential issues if the repository is updated with breaking changes.
Workflow ID: wflow_kzvRYlIMIaMA28rt
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8095e72 in 35 seconds
More details
- Looked at
126lines of code in5files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. py/tests/integration/harness_cli.py:67
- Draft comment:
Consider renaming the test functions to follow a consistent naming convention, such astest_<feature>_<method>. This will improve readability and maintainability. This comment applies to all test functions in this file andharness_sdk.py. - Reason this comment was not posted:
Confidence changes required:50%
The test function names inharness_cli.pyandharness_sdk.pyare inconsistent with the naming convention. They should follow a consistent pattern for clarity and maintainability.
2. py/tests/integration/harness_sdk.py:89
- Draft comment:
Consider renaming the test functions to follow a consistent naming convention, such astest_<feature>_<method>. This will improve readability and maintainability. This comment applies to all test functions in this file andharness_cli.py. - Reason this comment was not posted:
Confidence changes required:50%
The test function names inharness_cli.pyandharness_sdk.pyare inconsistent with the naming convention. They should follow a consistent pattern for clarity and maintainability.
3. py/tests/integration/harness_cli.py:73
- Draft comment:
Avoid using hardcoded document IDs. Consider parameterizing the document ID or retrieving it dynamically to improve maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The test functiontest_document_chunks_sample_file_cliinharness_cli.pyuses a hardcoded document ID. This can lead to maintenance issues if the document ID changes.
4. py/tests/integration/harness_sdk.py:91
- Draft comment:
Avoid using hardcoded document IDs. Consider parameterizing the document ID or retrieving it dynamically to improve maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The test functiontest_document_chunks_sample_file_sdkinharness_sdk.pyuses a hardcoded document ID. This can lead to maintenance issues if the document ID changes.
Workflow ID: wflow_mh5qV22I2QX0kbra
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on dc0fcb1 in 24 seconds
More details
- Looked at
104lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. py/tests/integration/harness_cli.py:88
- Draft comment:
Remove trailing whitespace for cleaner code. This issue is present in multiple places, such as lines 88, 90, 92, 94, 96, 98, 100, 102, 104, 106, and 108. - Reason this comment was not posted:
Confidence changes required:10%
The code contains multiple instances of trailing whitespace, which is unnecessary and should be removed for cleaner code.
2. py/tests/integration/harness_sdk.py:112
- Draft comment:
Remove trailing whitespace for cleaner code. This issue is present in multiple places, such as lines 112, 114, 116, 118, 120, 122, 124, 126, 128, 130, 132, 134, 136, and 138. - Reason this comment was not posted:
Confidence changes required:10%
The code contains multiple instances of trailing whitespace, which is unnecessary and should be removed for cleaner code.
Workflow ID: wflow_wM2hSNaAkKd9TfaT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c329e20 in 39 seconds
More details
- Looked at
41lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. .github/workflows/integration-test-workflow-debian.yml:100
- Draft comment:
The test functiontest_delete_and_reingest_sample_file_sdkis incorrectly named in the workflow file. It should betest_delete_and_reingest_sample_file_clito match the function inharness_sdk.py. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests a mismatch between the function name and the file path, but the diff shows a change that aligns the function name with the SDK path. This indicates that the change was intentional, and the function name is likely correct as 'test_delete_and_reingest_sample_file_sdk'. The comment seems to be based on an assumption rather than evidence from the diff.
I might be missing the context of the actual function names in the 'harness_sdk.py' file. If the function does not exist in 'harness_sdk.py', the comment could be valid. However, based on the diff alone, the change seems intentional.
The diff shows a clear change from CLI to SDK, suggesting the function name is correct. Without evidence from the 'harness_sdk.py' file, the comment appears speculative.
The comment should be deleted as it is speculative and not supported by the evidence in the diff, which shows an intentional change to the SDK path and function name.
2. py/tests/integration/harness_sdk.py:117
- Draft comment:
Ensure the document ID3e157b3a-8469-51db-90d9-52e7d896b49bis correct and consistent with the test data setup. This change might affect the test results if not aligned with the data setup. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_xAyaI13aUHUbpJ6I
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on f6683a6 in 48 seconds
More details
- Looked at
471lines of code in7files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. py/core/main/assembly/factory.py:301
- Draft comment:
Remove theprintstatement as it seems to be a leftover from debugging. - Reason this comment was not posted:
Confidence changes required:50%
Theprintstatement in thecreate_providersmethod is likely a leftover from debugging and should be removed for cleaner code.
2. py/sdk/auth.py:111
- Draft comment:
Remove theprintstatement as it seems to be a leftover from debugging. - Reason this comment was not posted:
Confidence changes required:50%
Theprintstatement in theupdate_usermethod is likely a leftover from debugging and should be removed for cleaner code.
Workflow ID: wflow_AvcSpwYcxOvdZPcs
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d149776 in 35 seconds
More details
- Looked at
168lines of code in5files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. py/core/main/api/ingestion_router.py:198
- Draft comment:
Consider re-adding the_validate_ingestion_config(ingestion_config)call to ensure the ingestion configuration is validated before proceeding. This helps prevent potential issues with invalid configurations. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is directly related to a change made in the diff, which is the removal of the_validate_ingestion_config(ingestion_config)call. The comment suggests that this removal could lead to issues if the ingestion configuration is not validated. This seems like a valid concern, as validation is typically important to ensure data integrity and prevent errors.
I might be missing the context of why the validation was removed. There could be a reason why the validation is no longer necessary, such as a refactor that moved validation elsewhere.
Even if the validation was moved elsewhere, the comment is still relevant to the change made in this diff. It highlights a potential issue that should be considered.
Keep the comment as it addresses a potential issue directly related to the change made in the diff.
2. py/core/main/api/auth_router.py:145
- Draft comment:
Consider re-adding the UUID validation foruser_idto ensure it is a valid UUID before proceeding with the update. This helps prevent potential issues with invalid user IDs. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The removal of the UUID validation could lead to issues if an invalid UUID is passed, as theupdate_userfunction expects a valid UUID. The comment is suggesting a code change that could prevent runtime errors, which aligns with the rules for actionable comments.
The comment assumes that the UUID validation is necessary without considering if theupdate_userfunction or other parts of the code handle invalid UUIDs. The comment might be unnecessary if there are other safeguards in place.
Given the context of the diff, it seems that the UUID validation was explicitly removed, which might indicate that the author has accounted for this elsewhere. However, without evidence of such handling, the comment remains relevant.
The comment is about a change made in the diff and suggests a potentially necessary code change to prevent issues with invalid UUIDs. It should be kept.
3. py/tests/integration/harness_sdk.py:484
- Draft comment:
Consider removing or commenting out the print statements used for debugging purposes to keep the test output clean. Use assertions for test validations instead. This applies to other test functions as well. - Reason this comment was not posted:
Confidence changes required:30%
The print statements in the test functions are useful for debugging but can clutter the output. It's better to use assertions for test validations and only print when necessary.
Workflow ID: wflow_H0lixVfd3U1s09cw
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8f9fde0 in 22 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. py/tests/integration/harness_sdk.py:6
- Draft comment:
Ensure that the change in port from 7274 to 7272 matches the intended server configuration. - Reason this comment was not posted:
Confidence changes required:50%
The change in the client URL from port 7274 to 7272 should be verified to ensure it aligns with the intended server configuration.
Workflow ID: wflow_1l4IDNhp3gWdWaEC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
emrgnt-cmplxty
added a commit
that referenced
this pull request
Oct 5, 2024
* add postgres to integration * add postgres to integration * up * rename * hardcode * add back postgres * add back postgres * add pgvector * add pgvector * add pgvector * add pgvector * add pgvector * add pgvector * add pgvector * tweak config docs * fix integration suite * fix integration suite * fix integration suite * up * up * up * up * up * up * up * up * up * update integration test * final user tests * final user tests
emrgnt-cmplxty
added a commit
that referenced
this pull request
Oct 5, 2024
* Feature/revive integration tests (#1343) * add postgres to integration * add postgres to integration * up * rename * hardcode * add back postgres * add back postgres * add pgvector * add pgvector * add pgvector * add pgvector * add pgvector * add pgvector * add pgvector * tweak config docs * fix integration suite * fix integration suite * fix integration suite * up * up * up * up * up * up * up * up * up * update integration test * final user tests * final user tests * Fix validation error on collection creation responses, remove unnecessary error on deletion (#1344) * Don't throw an error when deleting a collection with no documents, fix return on create collection, more js tests * Revert "Don't throw an error when deleting a collection with no documents, fix return on create collection, more js tests" This reverts commit f9f6ead. * Don't throw an error when deleting a collection with no documents, fix return on create collection, more js tests * more * Improve kg throughput (#1342) * try * up * up * space * add it in ingestion * rm ingestion * init * add semaphore * test * rm duplicates * kg_creation_settings * rm semaphores * increase conns * change it back * clean * up * up * up * up --------- Co-authored-by: --global=Shreyas Pimpalgaonkar <[email protected]> Co-authored-by: emrgnt-cmplxty <[email protected]> --------- Co-authored-by: Nolan Tremelling <[email protected]> Co-authored-by: Shreyas Pimpalgaonkar <[email protected]> Co-authored-by: --global=Shreyas Pimpalgaonkar <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Revives integration tests with updated workflows, test scripts, and configuration changes, along with OpenAPI schema and type hint updates.
integration-test-workflow-debian.ymlto include PostgreSQL setup and test execution.harness_cli.pyandharness_sdk.pyfor CLI and SDK test execution.AppConfigtoconftest.pyfor better test configuration management.openapi.jsonto include new response models for ingestion and update endpoints.ingestion_router.pyfor response models.get_entities()andget_triples()inkg.pyfromlisttodict.IngestionResponseandUpdateResponseinresponses.pyto handle optionaltask_id.This description was created by
for 8f9fde0. It will automatically update as commits are pushed.