Remove Her dependency #142

rzane · 2025-01-22T23:35:52Z

Summary

This PR removes vendored gems. We do this by replacing all usages of Her with Faraday.

This PR does contain breaking changes:

use_api is no longer available. If you need to customize the HTTP connection, you can assign TestTrack::Client.connection directly. Consider using TestTrack::Client.build_connection.
Her provides a bunch of methods for sending HTTP requests that are not implemented. This PR only implements HTTP requests that are necessary for test_track_rails_client to function. So, if consumers of this gem were invoking methods on TestTrack::Remote::* models directly, compatibility is not guaranteed.

None of these functionalities were documented, so the risk is pretty low.

Other Information

The test suite for this gem makes extensive use of stubbing. It's hard to be completely confident that this change represents parity.

/domain @Betterment/test_track_core
/platform @Betterment/test_track_core

rzane · 2025-01-24T16:58:55Z

app/models/concerns/test_track/persistence.rb

+  def save
+    return false unless valid?
+
+    persist!
+    true
+  rescue Faraday::UnprocessableEntityError
+    errors.add(:base, 'The HTTP request failed with a 422 status code')
+    false
+  end


Note: The 422 response contains the errors that occurred, and Her would normally assign that to the record. I don't think we're relying on that functionality, so I did the bare minimum here.

rzane · 2025-01-24T16:59:55Z

app/models/test_track/remote/assignment.rb

+  def initialize(...)
+    super
+    clear_changes_information
+  end


This resource uses ActiveModel::Dirty to implement variant_changed?. It'd probably be better to not rely on AM::Dirty, but I wanted to maintain the current behavior as much as possible.

smudge · 2025-01-29T17:55:28Z

app/models/concerns/test_track/persistence.rb

+  end
+
+  def save!
+    save or raise(ActiveModel::ValidationError, self)


rare or spotted in the wild! but is it worth using it?

Ha, this is one of those rare cases where you're supposed to use or. I restrict my usage to or raise and or return.

smudge · 2025-01-29T18:07:59Z

app/models/concerns/test_track/resource.rb

+  def _assign_attribute(name, value)
+    super
+  rescue ActiveModel::UnknownAttributeError
+    # Don't raise when we encounter an unknown attribute.


Because there was an existing test for this behavior, but also because it makes sense with the way APIs are versioned.

Also, if we don't allow unknown fields, any non-breaking change (e.g. new field is added) to the API will break the client.

We do have an API versioning strategy, so the addition of a new field could (and probably should) fall under a new version release. But I can understand not introducing additional strictness as part of this PR.

smudge · 2025-01-29T18:12:12Z

app/models/test_track/remote/assignment_detail.rb

-  def assigned_at
-    original = super
-    if original.blank? || !original.respond_to?(:in_time_zone)
-      nil
-    else
-      original.in_time_zone rescue nil # rubocop:disable Style/RescueModifier
-    end
-  end


I assume this is more or less what :datetime buys us here.

Yup, exactly. There's a test that covers this, too.

smudge · 2025-01-29T18:24:54Z

lib/test_track/client.rb

+
+    def request(method:, path:, fake:, body: nil, headers: nil)
+      # Ensure that fake responses are consistent with real responses
+      return JSON.parse(JSON.generate(fake)) if fake?


👍 so this is essentially the fakeable_her replacement API -- just pass in fake: with every request, which will be ignored if we're not in fake mode.

smudge · 2025-01-29T18:27:59Z

app/models/test_track/remote/identifier.rb

+  def visitor=(value)
+    @visitor = TestTrack::Remote::Visitor.new(value).to_visitor
  end


So the assignment here depends on assign_attributes (below) which is why this method needs to be public (?) even if it's not intended for anything external to be able to assign it.

Also value here is really hash with :id and :assignments -- wondering if there's a code/name change that would help communicate that. (It's basically private though, so it's more that it jumped out me as a public API and I was trying to figure out what uses it.)

Renamed to visitor_attributes

smudge · 2025-01-29T18:28:46Z

app/models/test_track/remote/split_config.rb

+  def self.destroy_existing(id)
+    TestTrack::Client.request(
+      method: :delete,
+      path: "api/v1/split_configs/#{id}",
+      fake: nil
+    )
+
+    nil
+  end


What calls this method?

test_track_rails_client/app/models/test_track/config_updater.rb

Lines 16 to 22 in 109bcb0

def drop_split(name)

TestTrack::Remote::SplitConfig.destroy_existing(name)

splits.except!(name.to_s)

persist_schema!

end

smudge · 2025-01-29T18:30:05Z

app/models/test_track/remote/split_detail.rb

@@ -39,4 +45,8 @@ def self.fake_variant_details
      }
    ]
  end
+
+  def variant_details=(values)
+    super(values.map(&:symbolize_keys))


Should a symbolize_keys or deep_symbolize_keys be baked into the way that TestTrack::Client.request returns results?

I looked into that. The Faraday::Response::Json middleware does allow you to specify parser_options: { symbolize_names: true }.

This project is been pretty inconsistent with usage of symbol versus string keys. Fakes mostly have symbol keys. Some of our stubs mix string/symbol keys. Splits have string keys (deep). Variant details use symbol keys (shallow).

Using symbolize_names actually caused more problems than it solved, for whatever reason.

smudge · 2025-01-29T18:32:04Z

app/models/test_track/remote/visitor_detail.rb

+  def assignment_details=(values)
+    assignment_details = values.map do |value|
+      TestTrack::Remote::AssignmentDetail.new(value)
+    end
+
+    super(assignment_details)
+  end


Cool, so just handling has_many mappings on assignment. 👍

smudge · 2025-01-29T22:09:48Z

lib/test_track_rails_client/assignment_helper.rb

-        weights: { variant.to_s => 100 },
-        feature_gate: split_name.end_with?('_enabled')
+        'weights' => { variant.to_s => 100 },
+        'feature_gate' => split_name.end_with?('_enabled')
      }
      assignments << { split_name:, variant: variant.to_s, unsynced: false }


still symbols here but needed strings above?

Yeah, this change might not be necessary now that I'm doing JSON.parse(JSON.generate(data)). Let me take a peek.

Okay, this change isn't actually necessary, so I could revert it, but I'd prefer to keep it.

This helper is building stubs. The stubbed return values should be consistent with the real return values.

TestTrack::Remote::Visitor.fake_instance_attributes returns a hash with deeply symbolized keys.

TestTrack::Remote::SplitRegistry.fake_instance_attributes returns a hash with deeply stringified keys.

Our usages of string vs symbol keys are really inconsistent.

$ spec/dummy/bin/rails c Loading development environment (Rails 8.0.1) [1] pry(main)> TestTrack::Remote::Visitor.fake_instance_attributes(nil) => {:id=>"fake_visitor_id", :assignments=>[{:split_name=>"split_1", :variant=>"true", :context=>"context", :unsynced=>false}, {:split_name=>"split_2", :variant=>"true", :context=>"context", :unsynced=>false}]} [2] pry(main)> TestTrack::Remote::SplitRegistry.fake_instance_attributes(nil) => {"splits"=>{"banner_color"=>{"weights"=>{"blue"=>100, "red"=>0, "white"=>0}, "feature_gate"=>false}, "buy_one_get_one_promotion_enabled"=>{"weights"=>{"false"=>100, "true"=>0}, "feature_gate"=>true}, "decided_split"=>{"weights"=>{"treatment"=>100, "control"=>0}, "feature_gate"=>false}}, "experience_sampling_weight"=>1}

smudge · 2025-01-29T22:11:57Z

lib/test_track/client.rb

+    def build_connection(url:, options: {})
+      Faraday.new(url) do |conn|
+        conn.use Faraday::Request::Json
+        conn.use Faraday::Response::Json, content_type: []


Remind me what content_type: [] does here -- it makes it so the server doesn't strictly need to return with a JSON content type for the response to be cast to JSON?

Right content_type: [] means "just assume it's JSON, even if the Content-Type header doesn't specify". It's more consistent with the old behavior from FaradayMiddleware::ParseJson.

I realize that the test track server is always going to specify the correct content type. The problem is that our tests using stub_request won't, and there's a lot of them.

smudge · 2025-01-29T22:14:17Z

lib/test_track/client.rb

+    attr_writer :connection
+
+    def fake?
+      !TestTrack.enabled?


Prior definition of faked? was:

def faked? !ENV["#{service_name.to_s.upcase}_ENABLED"] && (Rails.env.development? || Rails.env.test?) end

Unless we're changing the definition of enabled? somewhere, seems like we'd be changing the definition of fake?/faked? to ignore Rails.env when computing a default, right?

RemoteModel was overriding the definition from FakeableHer here:

test_track_rails_client/app/models/concerns/test_track/remote_model.rb

Lines 10 to 12 in 109bcb0

def faked?

!TestTrack.enabled?

end

rzane force-pushed the remove-her branch from 3dac3a6 to 041a99b Compare January 23, 2025 13:52

rzane added 29 commits January 24, 2025 11:07

Replace Her in SplitRegistry

e5b2bf3

Implement SplitConfig

d916189

Implement IdentifierType

478505b

Remove unused methods

7c5086c

Reimplement Identifier

b6c2599

Ignore unknown attributes

f2eebb7

Clean up Identifier#save

42dba0b

Implement Visitor

6f42ea2

Allow connection switching

1b02044

Avoid potential confustion with faraday_middleware

5750615

Replace VisitorDetail

f91a368

Replace AssignmentDetail

297e350

Convert Remote::Assignment

b2b4d4e

Convert SplitDetail

b4f9692

Convert AssignmentEvent

8a31499

Update FakeServer.reset

06794e4

Remove RemoteModel

1cc8c9f

Remove TestTrackApi

9a1250e

Remove vendored gems

e872e45

Remove Her

9874de4

Provide #request helper

1a642d1

Make sure fake responses are consistent with real responses

597073d

Remove unused dependencies

2cb115c

Tidy up #visitor=

60c6d25

Fix RuboCop

1224a75

Separate models from HTTP

effd79e

Always use .attribute

d7bca1d

Use ActiveModel::Model

55b0c65

Add spec for TestTrack::Resource

847b139

rzane added 8 commits January 24, 2025 11:44

Add convenience method for building a connection

57938f0

Add specs for persistence

bf68235

Clear changes information after initialize

bfe9cab

Ensure that middleware is in the correct order

e415238

Remove unnecessary require

0bde9fe

Avoid stubbing the test subject

3f3f1d9

Add a test for timeout

f39f96c

Fix RuboCop violations

86780f6

rzane force-pushed the remove-her branch from 33ddee1 to 86780f6 Compare January 24, 2025 16:45

rzane marked this pull request as ready for review January 24, 2025 16:46

rzane requested a review from smudge January 24, 2025 16:47

Bump version

acc66ff

rzane requested review from a team and removed request for smudge January 24, 2025 16:54

Clarify error message

94e8388

rzane commented Jan 24, 2025

View reviewed changes

smudge reviewed Jan 29, 2025

View reviewed changes

Rename to visitor_attributes

7f4c8ef

smudge reviewed Jan 29, 2025

View reviewed changes

rzane requested a review from smudge February 6, 2025 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Her dependency #142

Remove Her dependency #142

rzane commented Jan 22, 2025 •

edited

Loading

rzane Jan 24, 2025 •

edited

Loading

rzane Jan 24, 2025 •

edited

Loading

smudge Jan 29, 2025

rzane Jan 29, 2025

smudge Jan 29, 2025

rzane Jan 29, 2025

smudge Jan 29, 2025

smudge Jan 29, 2025

rzane Jan 29, 2025

smudge Jan 29, 2025

smudge Jan 29, 2025

rzane Jan 29, 2025

smudge Jan 29, 2025

rzane Jan 29, 2025

smudge Jan 29, 2025

rzane Jan 29, 2025 •

edited

Loading

smudge Jan 29, 2025

smudge Jan 29, 2025

rzane Feb 5, 2025

rzane Feb 5, 2025 •

edited

Loading

smudge Jan 29, 2025

rzane Feb 5, 2025 •

edited

Loading

smudge Jan 29, 2025 •

edited

Loading

rzane Feb 5, 2025

	def drop_split(name)
	TestTrack::Remote::SplitConfig.destroy_existing(name)

	splits.except!(name.to_s)

	persist_schema!
	end

Remove Her dependency #142

Are you sure you want to change the base?

Remove Her dependency #142

Conversation

rzane commented Jan 22, 2025 • edited Loading

Summary

Other Information

rzane Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

rzane Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzane Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzane Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzane Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

smudge Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rzane commented Jan 22, 2025 •

edited

Loading

rzane Jan 24, 2025 •

edited

Loading

rzane Jan 24, 2025 •

edited

Loading

rzane Jan 29, 2025 •

edited

Loading

rzane Feb 5, 2025 •

edited

Loading

rzane Feb 5, 2025 •

edited

Loading

smudge Jan 29, 2025 •

edited

Loading