-
Notifications
You must be signed in to change notification settings - Fork 5
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
maint: smoke test python app #91
Conversation
wait_for_ready_app ${CONTAINER_NAME} | ||
curl --silent "http://localhost:5000" | ||
docker-compose up --build --detach collector | ||
wait_for_ready_collector ${COLLECTOR_NAME} |
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.
This differs from most of our other smoke tests because this app just runs and exits - it doesn't have an endpoint. This ensures the collector is ready before the app runs and sends telemetry.
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.
Pulled this down locally and checked it out. It looks great! Just a few things:
Ran into the grpcio wheel build error, so I'd be interested in adding the following to the Dockerfile in hello-world example:
#pin the python version)
FROM python:3.10-alpine
#add g++ to the alpine build)
RUN apk add --no-cache gcc musl-dev python3-dev libffi-dev openssl-dev cargo g++
And for tidiness can we also add the smoke test artifacts not needed in version control (smoke-tests/**/data*.json
?) to the .gitignore?
Thank you!
} | ||
|
||
@test "BaggageSpanProcessor: key-values added to baggage appear on child spans" { | ||
result=$(span_attributes_for ${TRACER_NAME} | jq "select(.key == \"for_the_children\").value.stringValue") |
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.
just a note: is there somewhere in devleoping.md we should add a Smoke Tests section with the Make Commands and dependencies needed (brew install bats-core, brew install jq)
When I was going to run this for the first time it failed because I had dependencies to install.
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.
👍 I added these notes into the DEVELOPING.md
44bda62
to
195463c
Compare
@emilyashley thanks for the review! I've added some updates based on your suggestions, let me know what you think! |
Install `bats-core` and `jq` for local testing: | ||
|
||
```bash | ||
brew install bats-core |
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.
✨ thank you!
|
||
In the smoke-tests directory there exists a `docker-compose.yml` to run in Docker. | ||
|
||
Because each example uses the same port, either comment out the other apps in the docker-compose file, or specify the app to run: |
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.
great!
Which problem is this PR solving?
Short description of the changes
How to verify that this has the expected result
make smoke-sdk
runs and succeeds; artifacts are published with expected contents in circle