rocket_pants 5 by polinagogo · Pull Request #1 · indiegogo/rocket_pants

polinagogo · 2018-07-14T01:08:04Z

Pivotal: https://www.pivotaltracker.com/story/show/159012685
Monorail running this branch is green: https://github.com/indiegogo/monorail/pull/11639

Summary of Changes:

update ruby to supported version (2.0)
start replacing old mock behavior with supported mock behavior
get all specs to pass (half of them were failing)

Finished in 0.65248 seconds (files took 2.27 seconds to load)
186 examples, 0 failures

remove travis.yml
update kaminari integration to work with rocket_pants again
use our pinned version of active_model_serializers for the integration test
check in Gemfile.lock file
remove the check that throws 422 if exposed resources responds to invalid and returns true
update readme
cut a 5.0.0 tag, check that the monorail goes green with the updates so far
patch with Rails 5 compatible changes
confirm monorail is green with latest changes
figure out how to replace the proxy rr check in active_model_serializers_spec.rb
add ability to run on CI
update instrumentation so it works with skylight

Later:

fix deprecation warnings
make this repo a mirror instead of a fork?
update tag

polinagogo · 2018-07-17T00:41:22Z

spec/integration/active_model_serializers_spec.rb

-      mock.proxy(SerializerA).new(fish, rr_satisfy { |h| h[:url_options].present? }) { |r| r }
+      allow(TestController).to receive(:test_data) { fish }
+      allow(TestController).to receive(:test_options) { {:serializer => SerializerA} }
+      # mock.proxy(SerializerA).new(fish, rr_satisfy { |h| h[:url_options].present? }) { |r| r }


TODO: need to figure out how to do the equivalent of rr_satisfy { |h| h[:url_options].present? }

polinagogo · 2018-07-17T00:43:09Z

spec/integration/rspec_spec.rb

+
+  # TODO: need to rewrite all of this to support Rails 5
+  #
  # Hack to allow us to include the ActionController::TestCase::Behaviour module


there's a lot of ugly hacks in this test to mock on TestController, I think I need to switch everything to use ActionDispatch::Integration if I want these 4 tests to work in Rails 5x

polinagogo · 2018-07-17T00:45:17Z

Gemfile

-gem 'active_model_serializers', ENV['AMS_VERSION'] || '> 0.0'
+gem 'railties', '5.0.0'
+gem 'actionpack', '5.0.0'
+gem 'activerecord', '5.0.0'


testing this gem in 5.0.0 for now

polinagogo · 2018-07-17T00:47:15Z

lib/rocket_pants/test_helper.rb


-      if _default_version.present? && parameters[:version].blank? && parameters['version'].blank?
-        parameters[:version] = _default_version
+      if Rails::VERSION::MAJOR <= 4


this whole test_helper.rb is awful and just garbage. it's trying to prop up test controller implementation details in order to mock a controller.

IndieBarry · 2018-07-17T20:48:24Z

.circleci/config.yml

@@ -0,0 +1,39 @@
+version: 2


Is this the version of circle or the version number for our project?

this is what the circle documentation says to put -- I think it's the version of circle, because everyone on the internet has it set to 2 (circle 2)

IndieBarry · 2018-07-17T20:49:03Z

CHANGELOG.md


 **Please Note**: This change log only covers v1.3 forwards - apologies for anything missing prior to that.

+## Version 5.0.0


We're not going to just call it version 2? :-P

why would we do such a thing???

IndieBarry · 2018-07-17T20:50:22Z

CHANGELOG.md

+* Update test dependencies and make sure all tests pass
+* Remove TestHelper Rspec mixin
+* Removed be_exposed Rspec matcher due to egregious testing patterns
+* Removed Proxy Based testing in favore of testing default_serializer_options as unit test


spelling of favor

that was curtis

IndieBarry · 2018-07-17T20:50:51Z

Gemfile

+  gem 'pry'
+  gem 'pry-byebug'
+  # stable forked version of active_model_serializers in order to write an integration test
+  gem 'active_model_serializers', git: 'git@github.com:indiegogo/active_model_serializers.git', branch: '0-8-stable'


IndieBarry · 2018-07-17T20:52:08Z

Gemfile.lock

+    byebug (10.0.2)
+    coderay (1.1.2)
+    concurrent-ruby (1.0.5)
+    crack (0.4.3)


I thought you were removing crack, according to the comment in the CHANGELOG.md.

it looks like webmock uses crack unfortunately

IndieBarry · 2018-07-17T20:54:42Z

lib/rocket_pants.rb

-      env = Rails.env.to_s if defined?(Rails.env)
-      env ||= ENV['RAILS_ENV'].presence || ENV['RACK_ENV'].presence || "development"
-      ActiveSupport::StringInquirer.new env
+      if defined?(Rails.env) && Rails&.env


You shouldn't have to use &. since you just used Rails...

yeah I can remove it, np

IndieBarry · 2018-07-17T21:02:54Z

lib/rocket_pants.rb

+      if defined?(Rails.env) && Rails&.env
+        env = Rails.env.to_s
+      else
+        env ||= ENV['RAILS_ENV'].presence || ENV['RACK_ENV'].presence || "development"


You don't really need the ||= here. Perhaps if you want it to be clearer with the if-else, maybe do this instead:

env = if defined?(Rails.env) && Rails.env Rails.env.to_s else ENV["RAILS_ENV"].presence || ENV["RACK_ENV"].presence || "development" end

even though it seems to me that the original structure is fine and just add the addition Rails.env condition at the end of the original if.

IndieBarry · 2018-07-17T21:04:56Z

lib/rocket_pants/client.rb

    # @raise [RocketPants::Error] a generic error when the type is wrong, or a registered error subclass otherwise.
    def check_response_errors(response)
-      if !response.is_a?(Hash)
+      if !response.is_a?(HTTParty::Response)


So Rails 5 requires all the responses to be HTTParty::Response? That seems weird.

not exactly -- all responses were always HTTParty::Response BUT some versions of HTTParty responded true to is_a?(Hash). when I bumped HTTParty I had to update this

IndieBarry · 2018-07-17T21:06:32Z

lib/rocket_pants/controller/instrumentation.rb

+      ActiveSupport::Notifications.instrument("start_processing.action_controller", raw_payload.dup)

-      ActiveSupport::Notifications.instrument("process_action.rocket_pants", raw_payload) do |payload|
+      ActiveSupport::Notifications.instrument("process_action.action_controller", raw_payload) do |payload|


Why change these to the word action_controller? Seems like we'd want these to remain rocket_pants since that's where the notification is coming from?

I'll defer to Curtis for the explanation, but if it's rocket_pants we can't use Skylight instrumentation -- we're currently patching rocket_pants in the monorail to change this to action_controller

IndieBarry · 2018-07-17T21:09:11Z

lib/rocket_pants/controller/respondable.rb

        collection object, options
      else
-        if Respondable.invalid?(object)
-          error! :invalid_resource, object.errors


We don't depend on this behavior anywhere, do we?

other way around -- we depend on rocket pants NOT having this behavior

If we open source this project, will people appreciate this change or will e need to generalize it somehow?

IndieBarry · 2018-07-17T21:26:15Z

Seems mostly fine

sherrygogo · 2018-07-18T17:00:33Z

.circleci/config.yml

@@ -0,0 +1,39 @@
+version: 2


What are these circle config changes for?

oh nvm this is for the fork, ok carry on

sherrygogo · 2018-07-18T17:03:41Z

LGTM

- bump rocket pants version to 5.0.0 - remove travis.yml; replace with circleci yml - update ruby to supported version (2.3.5) - replace old mock behavior with newer rspec mock behavior - remove rr gem - get all specs to pass (half were originally failing) - update kaminari integraiton to work with rocket pants again - use pinned stable version of active_model_serializers for integration test - check in Gemfile.lock - remove check that raises if exposed resource returns to invalid and returns true - patch with Rails 5.0 compatible changes - update instrumentation so it works nicely with skylight

polina-munoz added 3 commits July 13, 2018 18:03

update ruby to supported version and add debugging gems

a818c11

change test mock to use rspec-mocks

4f26363

start fixing failing tests: caching_spec

011c3ce

polinagogo self-assigned this Jul 14, 2018

polinagogo force-pushed the ironbank/ps-fix-rocketpants-failing-tests branch from 011c3ce to ff21f39 Compare July 15, 2018 04:50

polinagogo changed the title ~~fix rocketpants failing tests~~ fix rocketpants Jul 16, 2018

polinagogo changed the title ~~fix rocketpants~~ rocketpants 5 Jul 16, 2018

polinagogo changed the title ~~rocketpants 5~~ rocket_pants 5 Jul 16, 2018

polinagogo force-pushed the ironbank/ps-fix-rocketpants-failing-tests branch from 5aef625 to e1c53fc Compare July 17, 2018 00:40

polinagogo commented Jul 17, 2018

View reviewed changes

IndieBarry reviewed Jul 17, 2018

View reviewed changes

sherrygogo reviewed Jul 18, 2018

View reviewed changes

polinagogo force-pushed the ironbank/ps-fix-rocketpants-failing-tests branch from f1b4d3f to 7892ca1 Compare July 18, 2018 17:28

polinagogo merged commit fb82605 into master Jul 18, 2018


		Please Note: This change log only covers v1.3 forwards - apologies for anything missing prior to that.

		## Version 5.0.0

Conversation

polinagogo commented Jul 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polinagogo Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IndieBarry commented Jul 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sherrygogo commented Jul 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

polinagogo commented Jul 14, 2018 •

edited

Loading

polinagogo Jul 17, 2018 •

edited

Loading