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

test:Migrate sidekiq tests to RSpec #1092

Merged
merged 4 commits into from
Jul 8, 2020
Merged

test:Migrate sidekiq tests to RSpec #1092

merged 4 commits into from
Jul 8, 2020

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Jun 23, 2020

This PR migrates our existing minitest Sidekiq tests to RSpec.

There's not much else say, to be honest 🙂.

@marcotc marcotc added the dev/testing Involves testing processes (e.g. RSpec) label Jun 23, 2020
@marcotc marcotc requested a review from a team June 23, 2020 22:45
@marcotc marcotc self-assigned this Jun 23, 2020
@marcotc marcotc requested a review from ericmustin July 7, 2020 19:29
ericmustin
ericmustin suggested changes Jul 8, 2020
View reviewed changes
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.

Nice work here! The migration from test (minitest) => spec (rspect) itself LGTM.

One small thing, I noticed in the circleci output that we're still running test:sidekiq in a few places in the Rakefile. Such as:

dd-trace-rb/Rakefile

Line 323 in 9acdc41

sh 'bundle exec appraisal contrib rake test:sidekiq'

I think we will need to update the Rakefile to spec:sidekiq to make sure these Rspec tests run in the builds.

@marcotc marcotc requested a review from ericmustin July 8, 2020 16:51
ericmustin
ericmustin previously approved these changes Jul 8, 2020
View reviewed changes
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.

Looks like there's still one left, but after that's updated should be good to go

dd-trace-rb/Rakefile

Line 201 in 80b9bd2

sh 'bundle exec appraisal contrib-old rake test:sidekiq'

also for some reason a test flaked so good to repush anyway?

@marcotc marcotc requested a review from ericmustin July 8, 2020 19:48
@marcotc
Copy link
Member Author

marcotc commented Jul 8, 2020

Thank you @ericmustin! Everything should be addressed.

We have two fixes for recent flaky tests: #1103 and #1091. #1091 has been merged, so I rebase this branch so that error doesn't come back.

ericmustin
ericmustin approved these changes Jul 8, 2020
View reviewed changes
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.

Lgtm 👍

@marcotc marcotc merged commit 64ec745 into master Jul 8, 2020
@marcotc marcotc deleted the test/sidekiq-minitest branch July 8, 2020 20:05
@marcotc marcotc added this to the 0.38.0 milestone Jul 8, 2020
This was referenced Jul 8, 2020
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericmustin ericmustin ericmustin approved these changes

Assignees

@marcotc marcotc

Labels
dev/testing Involves testing processes (e.g. RSpec)
Projects
None yet
Milestone
0.38.0
Development

Successfully merging this pull request may close these issues.
2 participants
@marcotc @ericmustin