Skip to content
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

Improved smoke tests #934

Merged
merged 12 commits into from
Aug 12, 2020
Merged

Improved smoke tests #934

merged 12 commits into from
Aug 12, 2020

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Aug 9, 2020

New smoke tests based on Testcontainers and OpenTelemetry Collector.

Target applications (Spring Boot, Wildfly and Play) are run inside Testcontainers with attached java agent. Agent is configured to send data to the Collector which uses fileexporter to write received spans to a json file. Tests then read that file and can assert anything they like about spans.

Partly addresses #298. What's left to be done there: write GitHub Action to build and publish target application distributions on their changes; support different java versions for target applications.

@iNikem
Copy link
Contributor Author

iNikem commented Aug 9, 2020

Build fails with "Previous attempts to find a Docker environment failed. Will not retry. Please see logs and check configuration". Hm... can we run Testcontainers in CircleCI at all??

@trask
Copy link
Member

trask commented Aug 9, 2020

Hm... can we run Testcontainers in CircleCI at all??

We are still using datadog/dd-trace-java-docker-build:latest to run our builds, so maybe that image doesn't have docker installed?

@iNikem
Copy link
Contributor Author

iNikem commented Aug 9, 2020

But we do want to use testcontainers? Not only for smoke tests, right? Should we hurry up our migration to Github Actions?

@trask
Copy link
Member

trask commented Aug 9, 2020

But we do want to use testcontainers? Not only for smoke tests, right? Should we hurry up our migration to Github Actions?

👍 from me

I'm feeling more confident about the move to Github Actions given open-telemetry/community#429 (comment)

@iNikem
Copy link
Contributor Author

iNikem commented Aug 9, 2020

But I don't think we can do it until we have working Gradle cache and that requires open-telemetry/community#392

@trask
Copy link
Member

trask commented Aug 9, 2020

why not? just too slow without the cache?

@iNikem
Copy link
Contributor Author

iNikem commented Aug 9, 2020

Yes. Current nightly takes ~35 minutes. I think this is too slow for PR build

@trask
Copy link
Member

trask commented Aug 10, 2020

Can we use local build cache for CI (at least for now) and save/restore that cache as part of the build?

@iNikem
Copy link
Contributor Author

iNikem commented Aug 10, 2020

We can try. I will get back to this in couple of days.

@iNikem
Copy link
Contributor Author

iNikem commented Aug 11, 2020

This PR also adds workflows for PR checks and for building applications for smoke tests (for now only for java 11). I also disabled smoke tests in CircleCI as the latter cannot run testcontainers. Github Actions take a lot of time for now, but Gradle cache is in place, so I will monitor how well will it work.

All in all, please review, let merge this and see what will happen.

@iNikem
Copy link
Contributor Author

iNikem commented Aug 11, 2020

GH Actions fail because there is no such images in this repo's packages yet. Will be solved shortly after merged.

I totally failed to split this into working independent pieces :(

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is awesome ❤️

Comment on lines +32 to +37
- name: Test with Gradle
uses: nick-invision/retry@v1
with:
command: ./gradlew testJava${{ matrix.java }} --stacktrace
timeout_minutes: 60
max_attempts: 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just checking that we want to do this retry on PRs (not just nightly)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still want to protect PRs from flaky test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just realized, this will slow down feedback (3x?) on real failing builds, which are common in PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly 3x, because already passed tests will not be re-run. But you are right. Will see how this goes...

.github/workflows/build-play-smoke-dist.yaml Outdated Show resolved Hide resolved
.github/workflows/build-springboot-smoke-dist.yaml Outdated Show resolved Hide resolved
smoke-tests/play/README.md Show resolved Hide resolved
smoke-tests/springboot/build.gradle Outdated Show resolved Hide resolved
Comment on lines +28 to +29
//We don't have support for Wildfly Undertow server yet.
//So this test just verifies that Wildfly has come up.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this should work now with real span verification, the reason it didn't before is because we were verifying from logging exporter and wildfly messed with stdout/stderr, i can try it out after the merge

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen any spans generated during this test

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks! Just one optional comment is I wonder if it's worth copying multiple gradle wrappers into the repo. It should be simple enough to have the smoke test projects not automatically built without having to duplicate Gradle?

@iNikem
Copy link
Contributor Author

iNikem commented Aug 12, 2020

Hm, indeed probably there is no need for several Gradle wrappers in one repo...

@iNikem
Copy link
Contributor Author

iNikem commented Aug 12, 2020

@anuraaga I still prefer to add Gradle wrappers into those projects. They are meant to be developed independently. You most probably will end up with a separate Idea project for them and it will be nice if they are self-contained. Gradle wrappers are small, less than 80K

@iNikem iNikem merged commit a222676 into open-telemetry:master Aug 12, 2020
@iNikem iNikem deleted the smoke-tests branch August 12, 2020 09:34
mabdinur pushed a commit to mabdinur/opentelemetry-java-instrumentation that referenced this pull request Aug 17, 2020
* Preping smoke tests

* Improved smoke tests

* Make it work in Linux

* Add workflow to run tests on PR

* Build Smoke tests

* Fix PR workflow trigger

* Fix smoke test apps trigger

* Disable smoke tests in CircleCI and let them run in Github Actions only

* Fix smoke test apps docker image names

* Polish
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants