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

migrate pipeline processor and filter tests to rspec #1133

Merged
merged 3 commits into from
Aug 13, 2020

Conversation

ericmustin
Copy link
Contributor

This PR moves over the Datadog::Pipeline, Datadog::Pipeline::SpanProcessor, and Datadog::Pipeline::SpanFilter minitest tests within the /tests folder to rspec spec's within the /spec folder. It tests what was covered in the minitest suite, specifically, the behavior of the trace pipeline, and the functionality of both OOTB processor and filter classes as well as arbitrary processors or filters as described here: https://github.com/DataDog/dd-trace-rb/blob/master/docs/GettingStarted.md#processing-pipeline

@ericmustin ericmustin requested a review from a team August 3, 2020 21:16
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Looks very good! Only a few minor suggestions.

Comment on lines 20 to 22

before do
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
before do
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 updated

Comment on lines 6 to 11
def generate_span(name, parent = nil)
Datadog::Span.new(nil, name).tap do |span|
span.parent = parent
end
end

Copy link
Member

Choose a reason for hiding this comment

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

You might want to move this method in to a shared file, like Sidekiq does here: https://github.com/DataDog/dd-trace-rb/tree/64ec7458bd9e6fe010382cdd655044a8aca7264f/spec/ddtrace/contrib/sidekiq/support

We normally add a support/ directory with shared files in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, updated

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

@ericmustin ericmustin merged commit e5956c7 into master Aug 13, 2020
@ericmustin ericmustin deleted the migrate_pipeline_tests_to_rspec branch August 13, 2020 12:33
@marcotc marcotc added this to the 0.40.0 milestone Sep 8, 2020
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.

2 participants