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.

Sign up for GitHub

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

Jump to bottom

Update images and enable leak detection, and split tests in CI to Unit, Integration and Web #299

Merged
merged 17 commits into from
Mar 8, 2019
Merged

Update images and enable leak detection, and split tests in CI to Unit, Integration and Web #299

merged 17 commits into from
Mar 8, 2019

Conversation

pawelchcki
Copy link
Contributor

@pawelchcki pawelchcki commented Feb 15, 2019 •
edited
Loading

Description

Use new Alpine based Debug images for most tests to ensure repeatable results.

Readiness checklist

@pawelchcki pawelchcki force-pushed the update_images_and_enable_leak_detection branch 3 times, most recently from 4c87807 to 4614f7e Compare February 20, 2019 17:51
@pawelchcki pawelchcki force-pushed the update_images_and_enable_leak_detection branch 27 times, most recently from 6c0e60d to 9544e24 Compare March 7, 2019 14:37
@pawelchcki pawelchcki force-pushed the update_images_and_enable_leak_detection branch from f82cd10 to baabd6d Compare March 7, 2019 14:44
@pawelchcki pawelchcki force-pushed the update_images_and_enable_leak_detection branch from baabd6d to 9df17ca Compare March 7, 2019 14:45
@pawelchcki pawelchcki force-pushed the update_images_and_enable_leak_detection branch 2 times, most recently from 681c038 to e714427 Compare March 7, 2019 15:14
@pawelchcki pawelchcki force-pushed the update_images_and_enable_leak_detection branch from e714427 to aa489d0 Compare March 7, 2019 15:21
@pawelchcki pawelchcki changed the title WIP: update images and enable leak detection Update images and enable leak detection, and split tests in CI to Unit, Integration and Web Mar 7, 2019
@pawelchcki pawelchcki force-pushed the update_images_and_enable_leak_detection branch from fb57168 to aa489d0 Compare March 7, 2019 16:48
labbati
labbati reviewed Mar 8, 2019
View reviewed changes
Copy link
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

I really love this. Especially the circle ci config is waaay better and execution much faster ❤️ .
Thanks for addressing this!

I added a couple of minor comments.

.circleci/config.yml
- <<: *IMAGE_DOCKER_MONGODB
- <<: *IMAGE_DOCKER_REQUEST_REPLAYER
environment:
DDAGENT_HOSTNAME: 127.0.0.1
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be DD_AGENT_HOST

.circleci/config.yml Outdated
integration_testsuite: "test-integrations-70"
image_tag: "ddtrace_alpine_php-7.0-debug"
- integration_tests:
name: "Intg tests 71"
Copy link
Member

Choose a reason for hiding this comment

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

Please don't hate me 😞 what about s/Intg/Integrations/g

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 agree with all my ❤️ but
I shortened it to make PHP version actually visible in Circle Ci UI. Which makes long names essentially useless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See Static Analy...
These changes actually started with Integration tests 54 and even Web integration tests 54 etc.
But this turned into a bunch of
Integration tes...
Integration tes...
Integration tes...
Integration tes...
Integration tes...
Web integratio...
Web integratio...
Web integratio...
Web integratio...
Web integratio...

I would have start with with version number but. The name of a job cannot start with a number in 2.1.

Maybe something like PHP 54 Integration tests could work WDYT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm actually I like that more than current short forms. Let me quickly change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WDYT?
image

Copy link
Member

Choose a reason for hiding this comment

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

I sort of hate that I asked you to change that 😄! I am ok with this, but feel free to go back to the initial proposal or to abbreviate Integrations again if you prefer 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right, I didn't like them either. But with naming I just usually try to use >first< thing that came to my mind, turns out 2nd idea was much better :D

.circleci/config.yml
@@ -1,4 +1,4 @@
version: 2.0
version: 2.1
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 we ended up eating the composer test-integration part (different from composer test-integrations-XX)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its there but its not immediately obvious. I remove the yaml reference in lieu of normal run devfinition to make it more obvious

labbati
labbati approved these changes Mar 8, 2019
View reviewed changes
Copy link
Member

@labbati labbati left a comment

Choose a reason for hiding this comment

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

👍 love it!

@pawelchcki pawelchcki merged commit 1642f69 into master Mar 8, 2019
@pawelchcki pawelchcki deleted the update_images_and_enable_leak_detection branch April 26, 2019 12:32
estringana added a commit that referenced this pull request Dec 4, 2023
@estringana
Exchange more types between extension and helper (#299)
7f32134
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@labbati labbati labbati approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pawelchcki @labbati