-
Notifications
You must be signed in to change notification settings - Fork 149
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
refactor: e2e tests using cloud trace v1 api. #2093
Conversation
"system-test:named-db:emulator:rest": "FIRESTORE_NAMED_DATABASE=test-db FIRESTORE_EMULATOR_HOST=localhost:8080 FIRESTORE_PREFER_REST=true mocha build/system-test --timeout 600000", | ||
"system-test:emulator:grpc": "FIRESTORE_EMULATOR_HOST=localhost:8080 mocha build/system-test --timeout 600000", | ||
"system-test:named-db:emulator:grpc": "FIRESTORE_NAMED_DATABASE=test-db FIRESTORE_EMULATOR_HOST=localhost:8080 mocha build/system-test --timeout 600000", | ||
"system-test:rest": "FIRESTORE_PREFER_REST=true mocha build/system-test --timeout 1200000", |
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.
why the timeout is extended? would this slow the full cycle down?
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.
yeah it'll slow down the start-to-finish time of the "integration tests CI". This PR adds 120 tests and the tests take a while because each test does at the very least one round trip to Firestore server, and at least 2-3 round-trips to cloud trace server (because the traces don't become available in the cloud trace server immediately, so we need to retry).
I am open to suggestions about how to reduce this cost. Maybe we should run the in-memory tests on PR CI and cloud-trace backend tests on a nightly basis.
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.
if only 10 minutes are added on top to run the whole system test, I think we could live with that comparing to having some part of the code not tested in PRs.
Regarding your question: For the time being, the reason this interface is here and not in Once we release this feature, we will move the Java does have Note that there is an inherent difference between OTEL in java and node.js. In Java the Firestore SDK accepts an instance |
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
Add end-to-end tests. Each end-to-end test enables tracing, performs one or more Firestore operations, and then queries the Cloud Trace backend to see if the expected trace and spans have been generated.