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
Split Detail Remote Model #26
Changes from 7 commits
4380210
6a5c160
4f17d02
be1eacb
1ce26a9
93c9dac
e481bc3
ae0c549
5db1905
ab9ceb1
8fa1c31
60932c0
289090b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
class Tt::Api::V1::SplitDetailsController < Tt::Api::V1::ApplicationController | ||
def show | ||
@split_detail = TestTrack::FakeServer.split_details | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
class TestTrack::Fake::SchemaLoading | ||
attr_reader :split_hash | ||
|
||
def initialize | ||
@split_hash = _split_hash | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we make this a public method called |
||
|
||
private | ||
|
||
def _split_hash | ||
if test_track_schema_yml.present? | ||
test_track_schema_yml[:splits] | ||
else | ||
{} | ||
end | ||
end | ||
|
||
def test_track_schema_yml | ||
unless instance_variable_defined?(:@test_track_schema_yml) | ||
@test_track_schema_yml = _test_track_schema_yml | ||
end | ||
@test_track_schema_yml | ||
end | ||
|
||
def _test_track_schema_yml | ||
YAML.load_file(test_track_schema_yml_path).with_indifferent_access | ||
rescue | ||
nil | ||
end | ||
|
||
def test_track_schema_yml_path | ||
ENV["TEST_TRACK_SCHEMA_FILE_PATH"] || "#{Rails.root}/db/test_track_schema.yml" | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
class TestTrack::Fake::Split | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this is actually SplitDetail, right? |
||
def self.instance | ||
@instance ||= new | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we just use |
||
|
||
def split_details | ||
@split_details ||= _split_details | ||
end | ||
|
||
private | ||
|
||
def _split_details | ||
split_hash = TestTrack::Fake::SchemaLoading.new.split_hash | ||
split_hash.each_with_object({}) do |(split_name, details), split_details| | ||
details_without_weight = details.except!("weights") | ||
split_details["name"] = split_name | ||
split_details.merge!(details_without_weight) | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
class TestTrack::Remote::SplitDetail | ||
include TestTrack::RemoteModel | ||
|
||
collection_path '/api/v1/split_details' | ||
|
||
attributes :name, :hypothesis, :assignment_criteria, :description, :owner, :location, :platform | ||
|
||
def self.from_name(name) | ||
# TODO: FakeableHer needs to make this faking a feature of `get` | ||
if faked? | ||
new(fake_instance_attributes(nil)) | ||
else | ||
get("/api/v1/split_details/#{name}") | ||
end | ||
end | ||
|
||
def self.fake_instance_attributes(_) | ||
{ | ||
name: "fake_split_name", | ||
hypothesis: "fake hypothesis", | ||
assignment_criteria: "fake criteria for everyone", | ||
description: "fake but still good description", | ||
owner: "fake owner", | ||
location: "fake activity", | ||
platform: "mobile" | ||
} | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
json.(@split_detail, "name", "hypothesis", "assignment_criteria", "description", "owner", "location", "platform") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
require 'rails_helper' | ||
|
||
RSpec.describe Tt::Api::V1::SplitDetailsController do | ||
let(:response_json) { JSON.parse(response.body) } | ||
|
||
describe '#show' do | ||
it 'returns fake split details' do | ||
get :show, format: :json | ||
expect(response_json).to eq TestTrack::FakeServer.split_details | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we want to list out the expected values? this seems like it's just asserting that a variable is equal to itself |
||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,23 @@ | ||
--- | ||
splits: | ||
buy_one_get_one_promotion_enabled: | ||
'false': 50 | ||
'true': 50 | ||
weights: | ||
'false': 50 | ||
'true': 50 | ||
hypothesis: "user will buy more hats with promotion" | ||
location: "hat store" | ||
platform: "desktop" | ||
owner: "hat makers" | ||
assignment_criteria: "user has bought one hat" | ||
description: "promotion test to see if users will buy more hats" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this actually getting dumped into the schema.yml now? i don't think that it is and i don't think that it should be. the schema.yml is only meant to track the critical details of splits, their variants, and the associated weighting. we don't want it to include any of the optional split detail data that is inputted through the admin ui. |
||
banner_color: | ||
blue: 34 | ||
red: 33 | ||
white: 33 | ||
weights: | ||
blue: 34 | ||
red: 33 | ||
white: 33 | ||
hypothesis: "user will interact more with blue banner" | ||
location: "home screen" | ||
platform: "mobile" | ||
owner: "mobile team" | ||
assignment_criteria: "user has mobile app" | ||
description: "banner test to see if users will interact more" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
require 'rails_helper' | ||
|
||
RSpec.describe TestTrack::Fake::Split do | ||
subject { Class.new(described_class).instance } | ||
|
||
context 'when splits exist' do | ||
describe '#split_details' do | ||
it 'returns a hash of split details for a split' do | ||
expect(subject.split_details["name"]).to eq "banner_color" | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
require 'rails_helper' | ||
|
||
RSpec.describe TestTrack::Remote::SplitDetail do | ||
before do | ||
stub_request(:get, url) | ||
.with(basic_auth: %w(dummy fakepassword)) | ||
.to_return(status: 200, body: { | ||
name: "fake_split_name_from_server", | ||
hypothesis: "hypothesis from a real server", | ||
assignment_criteria: "criteria about real not fake users", | ||
description: "description about a very real test", | ||
owner: "best owner ever", | ||
location: "the homepage above the fold", | ||
platform: "mobile" | ||
}.to_json) | ||
end | ||
|
||
describe ".find" do | ||
let(:url) { "http://testtrack.dev/api/v1/split_details/fake_split_name_from_server" } | ||
subject { described_class.find("fake_split_name_from_server") } | ||
|
||
it "loads split details with fake instance attributes" do | ||
expect(subject.name).to eq("fake_split_name") | ||
expect(subject.hypothesis).to eq("fake hypothesis") | ||
expect(subject.assignment_criteria).to eq("fake criteria for everyone") | ||
expect(subject.description).to eq("fake but still good description") | ||
expect(subject.owner).to eq("fake owner") | ||
expect(subject.location).to eq("fake activity") | ||
expect(subject.platform).to eq("mobile") | ||
end | ||
|
||
it "fetches attributes from the test track server when enabled" do | ||
with_test_track_enabled do | ||
expect(subject.name).to eq("fake_split_name_from_server") | ||
expect(subject.hypothesis).to eq("hypothesis from a real server") | ||
expect(subject.assignment_criteria).to eq("criteria about real not fake users") | ||
expect(subject.description).to eq("description about a very real test") | ||
expect(subject.owner).to eq("best owner ever") | ||
expect(subject.location).to eq("the homepage above the fold") | ||
expect(subject.platform).to eq("mobile") | ||
end | ||
end | ||
end | ||
|
||
describe ".from_name" do | ||
subject { described_class.from_name("clown_id") } | ||
let(:url) { "http://testtrack.dev/api/v1/split_details/clown_id" } | ||
|
||
it "loads split details with instance attributes" do | ||
expect(subject.name).to eq("fake_split_name") | ||
end | ||
|
||
it "fetches attributes from the test track server when enabled" do | ||
with_test_track_enabled do | ||
expect(subject.name).to eq("fake_split_name_from_server") | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could probably just name this
Schema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, worth it to make this a singleton? otherwise if we have multiple uses it'll load the same schema file twice, right?