-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[CHORE] JS client CI update #5371
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
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
JS Client CI & Integration Improvements and Migration to clients/new-js This PR rewires the CI pipeline and development scripts for the Chroma JavaScript client, switching focus to the new JS implementation in Key Changes• Replaces Affected Areas• This summary was automatically generated by @propel-code-bot |
2 Jobs Failed: PR checks / all-required-pr-checks-passedStep "Decide whether the needed jobs succeeded or failed" from job "all-required-pr-checks-passed" is failing. The last 20 log lines are:
PR checks / JavaScript client tests / testStep "Test" from job "JavaScript client tests / test" is failing. The last 20 log lines are:
Summary: 1 successful workflow, 1 failed workflow
Last updated: 2025-08-29 23:38:00 UTC |
export const client = createClient( | ||
createConfig<ClientOptions>({ |
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.
[CriticalError]
Consider validating the default port configuration. The client defaults to 'http://localhost:8000' but the server starts on '127.0.0.1:8000'. This inconsistency could cause connection failures.
export const client = createClient(
createConfig<ClientOptions>({
baseUrl: "http://127.0.0.1:8000", // Match server host
throwOnError: true,
}),
);
Alternatively, ensure both use the same host consistently throughout the codebase.
bin/ts-integration-test.sh
Outdated
# Start the Chroma server in the background | ||
echo "Building and starting Chroma server..." | ||
cargo build --bin chroma | ||
cargo run --bin chroma run & | ||
SERVER_PID=$! | ||
|
||
# Wait for the server to be ready | ||
wait_for_server | ||
|
||
# Install dependencies | ||
cd clients/js | ||
|
||
# Generate the JS client | ||
# Generate API | ||
echo "Generating JS client..." | ||
pnpm genapi | ||
cd clients/new-js/packages/chromadb | ||
pnpm genapi -- --use-localhost | ||
|
||
# Cleanup: kill the server process | ||
kill $SERVER_PID | ||
|
||
pnpm prettier | ||
|
||
# run git diff and check if packages/chromadb-core/src/generated/ has changed | ||
# run git diff and check if packages/chromadb/src/api/ has changed | ||
echo "Checking for changes in generated client..." | ||
if ! git diff --quiet --exit-code packages/chromadb-core/src/generated/; then | ||
if ! git diff --quiet --exit-code src/api/; then | ||
echo "Error: Generated JS client has changed. Please commit the changes." | ||
git diff packages/chromadb-core/src/generated/ | cat | ||
git diff src/api/ | cat | ||
exit 1 | ||
fi | ||
echo "No changes detected in generated client." | ||
|
||
# Install dependencies and run tests | ||
# Run tests | ||
export DEFAULT_CHROMA_INSTANCE_URL="http://localhost:8000" | ||
export DEFAULT_CHROMA_INSTANCE_HOST="127.0.0.1" | ||
|
||
echo "Running tests..." | ||
pnpm -r test --verbose | ||
pnpm -r test | ||
|
||
# Cleanup: kill the server process | ||
kill $SERVER_PID |
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.
[BestPractice]
The server process is started in the background, but the kill $SERVER_PID
command is at the very end of the script. Since set -e
is active, if any command fails before the end of the script (e.g., pnpm genapi
, git diff
, or pnpm test
), the script will exit immediately, leaving the server process running. This could cause issues in CI environments by consuming resources.
Using a trap
will ensure the server process is cleaned up regardless of how the script exits.
bin/ts-integration-test.sh
Outdated
pnpm -r test | ||
|
||
# Cleanup: kill the server process | ||
kill $SERVER_PID |
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.
[BestPractice]
For a more robust cleanup, consider killing the entire process group instead of just the single process. The cargo run
command can spawn child processes that might be orphaned if only the parent process is killed, which could lead to resource leaks on the CI runner.
Using kill -- -$SERVER_PID
sends the signal to all processes in the process group, ensuring a cleaner shutdown.
kill $SERVER_PID | |
kill -- -$SERVER_PID |
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
# Function to check if server is ready | ||
check_server() { | ||
curl -s http://localhost:8000/openapi.json > /dev/null | ||
return $? | ||
} |
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.
[BestPractice]
To improve consistency and avoid potential issues with localhost
resolving to IPv6 addresses in some environments, it would be better to use variables for the host and port and use 127.0.0.1
consistently. This also makes the script easier to configure if the port needs to be changed in the future.
bin/ts-integration-test.sh
Outdated
export DEFAULT_CHROMA_INSTANCE_URL="http://localhost:8000" | ||
export DEFAULT_CHROMA_INSTANCE_HOST="127.0.0.1" |
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.
[BestPractice]
Using the variables defined at the top of the script will ensure consistency.
await rm("../../../../chroma_integration_test_tmp_dir/", { | ||
recursive: true, | ||
force: true, | ||
}); |
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.
[BestPractice]
Using a relative path with ../../../../
to locate a directory from the project root can be brittle. If this script's location changes in the future, this path will break. A more robust approach is to construct the path programmatically using path.join
and __dirname
.
Description of changes
Updating the CI for JS to test the new JS client on PRs. The integration script for JS (
bin/ts-integration-test.sh
) will now correctly generate types from the server on every PR, and will fail if changes were not committed. It will also use the updated tests and code fromclients/new-js
. The API generation script and tests now have the ability to skip spinning up the server if one is already available (set up by the integration test in this case).Test plan
How are these changes tested?
pytest
for python,yarn test
for js,cargo test
for rust