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

Convert to rspec for non-sample app specs #128

Merged
merged 7 commits into from
Jun 24, 2024
Merged

Convert to rspec for non-sample app specs #128

merged 7 commits into from
Jun 24, 2024

Conversation

ssunday
Copy link
Contributor

@ssunday ssunday commented Jun 24, 2024

Convert to rspec for tests (broken out from #127 for sanity)

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

ENV['RAILS_ENV'] = 'test'
require_relative 'dummy/config/application'
require 'rspec/rails'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

necessary order change

@ssunday
Copy link
Contributor Author

ssunday commented Jun 24, 2024

Is there a reason to keep supporting end of life-d rails/ruby versions...? I feel like they should just be cut out/not officially supported.

Will do the rubocop fixes.

@mullermp
Copy link
Contributor

We can definitely consider a major version bump and drop old versions of Rails/Ruby. The SDK supports 2.5+ and in general we try to keep support where we can. We should keep the same matrix paradigm where we test every minor version of rails with ruby.

@mullermp
Copy link
Contributor

I think it's reasonable to start at Rails 7, and Ruby 2.7 (minimum of Rails 7)

@ssunday
Copy link
Contributor Author

ssunday commented Jun 24, 2024

@mullermp 👍🏻 Works for me. I'll make that change to the matrix and do the other things

@mullermp
Copy link
Contributor

mullermp commented Jun 24, 2024

We should update the CI, README, VERSION, and gemspec for the major version bump possibly in yet another PR... I'd be happy to merge that quickly to unblock this one.

@ssunday
Copy link
Contributor Author

ssunday commented Jun 24, 2024

#129 let's see if I did it right

Copy link
Contributor

@mullermp mullermp left a comment

Choose a reason for hiding this comment

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

Next I think it's just rubocop and see what CI does.

aws-sdk-rails.gemspec Show resolved Hide resolved
@ssunday
Copy link
Contributor Author

ssunday commented Jun 24, 2024

rubocop is running against all the old spec files since I renamed/changed all of them. I can fix all of them


spec.required_ruby_version = '>= 2.5'
spec.required_ruby_version = '>= 2.7'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI @mullermp

Copy link
Contributor

@mullermp mullermp left a comment

Choose a reason for hiding this comment

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

There's some in the rubocop todo if you are feeling adventurous. If not, I think we can move on to the other PR now.

@mullermp mullermp merged commit c17713a into aws:main Jun 24, 2024
17 of 18 checks passed
@ssunday ssunday deleted the convert-to-rspec branch June 24, 2024 18:18
@mullermp mullermp changed the title [TEST] Convert to rspec for non-sample app specs Convert to rspec for non-sample app specs Jun 24, 2024
@mullermp
Copy link
Contributor

I don't know if I merged this prematurely or not. I see in the other PR actually that you've converted actual tests to use RSpec.describe and new syntax! Let me know if that's the case.

@ssunday
Copy link
Contributor Author

ssunday commented Jun 24, 2024

@mullermp I can just do that in the other PR/keep changes, when I was working on it there I didn't realize I didn't have to to MVP get it done lolol.

@ssunday
Copy link
Contributor Author

ssunday commented Jun 24, 2024

@mullermp this is good to get merged, IMO.

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