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

test:Migrate span tests to RSpec #1149

Merged
merged 2 commits into from
Aug 21, 2020
Merged

test:Migrate span tests to RSpec #1149

merged 2 commits into from
Aug 21, 2020

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Aug 18, 2020

Migrate tests from test/span_test.rb to spec/ddtrace/span_spec.rb. Also added a few extra assertions that were missing, specially for #parent= and #set_error.

@marcotc marcotc added the dev/testing Involves testing processes (e.g. RSpec) label Aug 18, 2020
@marcotc marcotc added this to the 0.40.0 milestone Aug 18, 2020
@marcotc marcotc requested a review from a team August 18, 2020 22:50
@marcotc marcotc self-assigned this Aug 18, 2020
ericmustin
ericmustin previously approved these changes Aug 19, 2020
Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

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

🎉 👍

@@ -187,7 +187,6 @@

context 'is a block' do
it 'yields to the error block and raises the error' do
expect_any_instance_of(Datadog::Span).to_not receive(:set_error)
Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out a specific combination of any_instance_of and JRuby causes issues like this: https://app.circleci.com/pipelines/github/DataDog/dd-trace-rb/1694/workflows/b0a5b0af-711b-4aca-81db-3c99b1d504e5/jobs/81614

I've opened a rspec-mocks issue with this report: rspec/rspec-mocks#1338
I wasn't able to confidently fix it, unfortunately 😕

Copy link

Choose a reason for hiding this comment

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

Appreciate if you could respond in your ticket. Unfortunately, most probably nobody is going to fix it for you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @pirj, I'll follow up on the ticket, but long story short: I dedicated around 2 days trying to fix it in rspec-mocks but I wasn't able to.

Copy link

Choose a reason for hiding this comment

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

Thanks a lot. I understand, if it's tolerable on your side, it might not worth the effort. However, if you decide to proceed, I'll be happy to provide any support.

Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

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

gotcha...weird bug, otherwise lgtm

@marcotc marcotc merged commit 0e00961 into master Aug 21, 2020
@marcotc marcotc added this to Merged & awaiting release in Active work via automation Aug 21, 2020
@marcotc marcotc deleted the test/migrate-span branch August 21, 2020 17:59
@delner delner moved this from Merged & awaiting release to Released in Active work Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev/testing Involves testing processes (e.g. RSpec)
Projects
Active work
  
Released
Development

Successfully merging this pull request may close these issues.

None yet

3 participants