From 4a87798b2526920c2357a61074770202ecf1d0aa Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 17 Feb 2014 15:46:11 -0800 Subject: [PATCH 1/9] Some skeleton tests --- spec/web_spec.rb | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/spec/web_spec.rb b/spec/web_spec.rb index 6b61ae6..ccab9d0 100644 --- a/spec/web_spec.rb +++ b/spec/web_spec.rb @@ -2,6 +2,8 @@ require 'spec_helper' require './lib/descartes/web' +require './lib/descartes/models/metrics' + describe Descartes::Web do include Rack::Test::Methods @@ -10,5 +12,14 @@ def app Descartes::Web end - it 'should have some tests' + it 'should have more tests' + + describe '/metrics/' do + context 'json' do + it 'loads entire cache without any params' + it 'loads specific pages when requested via :page' + it 'changes page size optionally via :limit' + it 'defaults to page size of 50 metrics' + end + end end From dadb972f9274891ed7e3cc025051720d21c62aa7 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 17 Feb 2014 17:10:12 -0800 Subject: [PATCH 2/9] Punch-through oauth when testing API calls --- Gemfile | 3 ++- Gemfile.lock | 12 +++++++++--- spec/web_spec.rb | 6 ++++++ 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/Gemfile b/Gemfile index 93991be..f559d9c 100644 --- a/Gemfile +++ b/Gemfile @@ -24,5 +24,6 @@ gem "dotenv" group :development do gem "foreman" gem "pry" - gem "rack-test" + # 0.6.2 on rubygems is too old, lacks things like 'env' + gem "rack-test", :github => "brynary/rack-test", :ref => "8cdb86e1d7" end diff --git a/Gemfile.lock b/Gemfile.lock index 5b885d8..cf2e797 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,11 @@ +GIT + remote: git://github.com/brynary/rack-test.git + revision: 8cdb86e1d710194ce45f439915707db7fb2b27e5 + ref: 8cdb86e1d7 + specs: + rack-test (0.6.2) + rack (>= 1.0) + GEM remote: http://rubygems.org/ specs: @@ -54,8 +62,6 @@ GEM rack-protection (1.5.2) rack rack-ssl-enforcer (0.2.6) - rack-test (0.6.2) - rack (>= 1.0) rake (10.1.1) redis (3.0.6) rest-client (1.6.7) @@ -117,7 +123,7 @@ DEPENDENCIES pry rack-canonical-host rack-ssl-enforcer - rack-test + rack-test! rake rest-client rspec diff --git a/spec/web_spec.rb b/spec/web_spec.rb index ccab9d0..6033719 100644 --- a/spec/web_spec.rb +++ b/spec/web_spec.rb @@ -16,6 +16,12 @@ def app describe '/metrics/' do context 'json' do + before :each do + # Be an API request. + env 'HTTP_X_DESCARTES_API_TOKEN', ENV['API_TOKEN'] + header 'Accept', 'application/json' + end + it 'loads entire cache without any params' it 'loads specific pages when requested via :page' it 'changes page size optionally via :limit' From 85e2bbd029e2087c36f3e6a4ad20fb967f5e999e Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 17 Feb 2014 17:35:39 -0800 Subject: [PATCH 3/9] Test existing behavior --- spec/web_spec.rb | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/spec/web_spec.rb b/spec/web_spec.rb index 6033719..9bc451a 100644 --- a/spec/web_spec.rb +++ b/spec/web_spec.rb @@ -1,3 +1,4 @@ +require 'rspec' require 'rack/test' require 'spec_helper' @@ -5,6 +6,24 @@ require './lib/descartes/models/metrics' +# Meh? +class Metric + def self.paths=(paths) + @@paths = paths + end +end + +# Don't see a way to do this natively in Rack. 'response.json' idea stolen +# from python-requests lib. +module Rack + class Response + def json + JSON.parse(body) + end + end +end + + describe Descartes::Web do include Rack::Test::Methods @@ -20,9 +39,15 @@ def app # Be an API request. env 'HTTP_X_DESCARTES_API_TOKEN', ENV['API_TOKEN'] header 'Accept', 'application/json' + # Fake metric paths + Metric.paths = 1.upto(100).to_a.map {|x| "metric.#{x}"} + end + + it 'without any params, loads entire cache' do + get '/metrics/' + expect(last_response.json.size).to eq(100) end - it 'loads entire cache without any params' it 'loads specific pages when requested via :page' it 'changes page size optionally via :limit' it 'defaults to page size of 50 metrics' From 1d70b82cbfc4880c3301e99b9fb10c714d4849fd Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 17 Feb 2014 17:40:15 -0800 Subject: [PATCH 4/9] Flesh out failing tests --- spec/web_spec.rb | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/spec/web_spec.rb b/spec/web_spec.rb index 9bc451a..583c845 100644 --- a/spec/web_spec.rb +++ b/spec/web_spec.rb @@ -48,9 +48,29 @@ def app expect(last_response.json.size).to eq(100) end - it 'loads specific pages when requested via :page' - it 'changes page size optionally via :limit' - it 'defaults to page size of 50 metrics' + it 'loads specific pages when requested via :page' do + get '/metrics/', :page => 1 + result = last_response.json + expect(result.size).to eq(50) + expect(result.first).to eq('metric.1') + expect(result.last).to eq('metric.50') + end + + it 'changes page size optionally via :limit' do + get '/metrics/', :page => 2, :limit => 25 + result = last_response.json + expect(result.size).to eq(25) + expect(result.first).to eq('metric.26') + expect(result.last).to eq('metric.50') + end + + it 'assumes page 1 when :limit given & no :page' do + get '/metrics/', :limit => 25 + result = last_response.json + expect(result.size).to eq(25) + expect(result.first).to eq('metric.1') + expect(result.last).to eq('metric.25') + end end end end From c48cd8dc1e624eaea7fbfcc7217385fb47ab2112 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 17 Feb 2014 17:47:04 -0800 Subject: [PATCH 5/9] Refactor & polish tests somewhat --- spec/web_spec.rb | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/spec/web_spec.rb b/spec/web_spec.rb index 583c845..e0affe5 100644 --- a/spec/web_spec.rb +++ b/spec/web_spec.rb @@ -40,36 +40,35 @@ def app env 'HTTP_X_DESCARTES_API_TOKEN', ENV['API_TOKEN'] header 'Accept', 'application/json' # Fake metric paths - Metric.paths = 1.upto(100).to_a.map {|x| "metric.#{x}"} + Metric.paths = 1.upto(100).to_a + end + + def assert_array(size, start=nil, end_=nil) + expect(last_response.ok?).to be_true # Fail fast + result = last_response.json + expect(result.size).to eq(size) + expect(result.first).to(eq(start)) unless start.nil? + expect(result.last).to(eq(end_)) unless end_.nil? end it 'without any params, loads entire cache' do get '/metrics/' - expect(last_response.json.size).to eq(100) + assert_array 100 end it 'loads specific pages when requested via :page' do get '/metrics/', :page => 1 - result = last_response.json - expect(result.size).to eq(50) - expect(result.first).to eq('metric.1') - expect(result.last).to eq('metric.50') + assert_array 50, 1, 50 end it 'changes page size optionally via :limit' do get '/metrics/', :page => 2, :limit => 25 - result = last_response.json - expect(result.size).to eq(25) - expect(result.first).to eq('metric.26') - expect(result.last).to eq('metric.50') + assert_array 25, 26, 50 end it 'assumes page 1 when :limit given & no :page' do get '/metrics/', :limit => 25 - result = last_response.json - expect(result.size).to eq(25) - expect(result.first).to eq('metric.1') - expect(result.last).to eq('metric.25') + assert_array 25, 1, 25 end end end From 1a420093cfc16a0fe402e431f0e754344f8934bb Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 17 Feb 2014 18:05:10 -0800 Subject: [PATCH 6/9] Implement API-level pagination such that tests pass Conflicts: lib/descartes/routes/metrics.rb --- lib/descartes/routes/metrics.rb | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/descartes/routes/metrics.rb b/lib/descartes/routes/metrics.rb index 81f01da..9b44486 100644 --- a/lib/descartes/routes/metrics.rb +++ b/lib/descartes/routes/metrics.rb @@ -3,9 +3,22 @@ class Web < Sinatra::Base get '/metrics/?' do if request.xhr? + # No params -> just return everything. + metrics = if params.empty? + Metric.all + # Params -> paginate, depending. + else + page = (params['page'] || 1).to_i + size = (params['limit'] || 50).to_i + start = (page - 1) * size + end_ = start + size + Metric.all[start...end_] + end + + # Response content_type "application/json" status 200 - Metric.all.to_json + metrics.to_json else haml :'metrics/index', :locals => { :title => "Descartes - Metrics List", :cache_age => MetricCacheStatus.first.updated_at.to_s } end @@ -22,4 +35,4 @@ class Web < Sinatra::Base end end -end \ No newline at end of file +end From 2f01e6a43912e6696f4d4dd4e1f3bef13115cf51 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Mon, 17 Feb 2014 18:15:00 -0800 Subject: [PATCH 7/9] Fix test/impl to use tighter params test. Real webapp behavior has non-empty params. --- lib/descartes/routes/metrics.rb | 2 +- spec/web_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/descartes/routes/metrics.rb b/lib/descartes/routes/metrics.rb index 9b44486..630fe8d 100644 --- a/lib/descartes/routes/metrics.rb +++ b/lib/descartes/routes/metrics.rb @@ -4,7 +4,7 @@ class Web < Sinatra::Base get '/metrics/?' do if request.xhr? # No params -> just return everything. - metrics = if params.empty? + metrics = if %w(page limit).map {|x| params.include?(x)}.none? Metric.all # Params -> paginate, depending. else diff --git a/spec/web_spec.rb b/spec/web_spec.rb index e0affe5..b7ea265 100644 --- a/spec/web_spec.rb +++ b/spec/web_spec.rb @@ -51,8 +51,8 @@ def assert_array(size, start=nil, end_=nil) expect(result.last).to(eq(end_)) unless end_.nil? end - it 'without any params, loads entire cache' do - get '/metrics/' + it 'without page/limit params, loads entire cache' do + get '/metrics/', :_ => 12345 # ape real webapp behavior assert_array 100 end From 817e5181ea5a63576816b80b86d395f12473eee6 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Tue, 18 Feb 2014 08:58:36 -0800 Subject: [PATCH 8/9] First stab at webapp/JS side of pagination; 1st page load Conflicts: lib/descartes/public/js/list-metrics.js --- lib/descartes/public/js/list-metrics.js | 30 ++++++++++++++++--------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/lib/descartes/public/js/list-metrics.js b/lib/descartes/public/js/list-metrics.js index 994e2df..4ec1d9e 100644 --- a/lib/descartes/public/js/list-metrics.js +++ b/lib/descartes/public/js/list-metrics.js @@ -37,22 +37,30 @@ var addMetricsToolbar = function(metric) { html('12'); }; -// Grab configuration blob and construct our graph urls -var renderGraphs = function() { - // Load index.json at first load and cache as myLoadedMetrics - // Use cached myLoadedMetrics for infinite scrolling - if (myLoadedMetrics.length > 0) { - // Use cached myLoadedMetrics - renderSparklines(); - } else { - // Load index.json and cache in myLoadedMetrics +// Load a page of metrics from backend +var loadMetricsPage = function(page) { return $.ajax({ accepts: {'json': 'application/json'}, cache: false, dataType: 'json', error: function(xhr, textStatus, errorThrown) { console.log(errorThrown); }, - url: '/metrics/' - }).done(function(d) { + // Load first page by default + url: '/metrics/', + data: {limit: myMetricsPerPage, page: page} + }); +} + + +// Render current page of graphs, querying 1st page if none stored +var renderGraphs = function() { + // loaded metrics non-empty? just render 'em + if (myLoadedMetrics.length > 0) { + // Use cached myLoadedMetrics + renderSparklines(); + // empty? grab 1st page. + } else { + $('div.loading').removeClass('hidden'); + return loadMetricsPage(1).done(function(d) { if (d.length === 0) { console.log('No metrics found'); } else { From 67150198b344c9dd2d388f85748b88bb0ad4d9e5 Mon Sep 17 00:00:00 2001 From: Jeff Forcier Date: Tue, 18 Feb 2014 12:24:16 -0800 Subject: [PATCH 9/9] Update metrics list page JS to use backend pagination --- lib/descartes/public/js/list-metrics.js | 29 +++++++++++++++++-------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/lib/descartes/public/js/list-metrics.js b/lib/descartes/public/js/list-metrics.js index 4ec1d9e..d91489d 100644 --- a/lib/descartes/public/js/list-metrics.js +++ b/lib/descartes/public/js/list-metrics.js @@ -75,15 +75,18 @@ var renderSparklines = function() { // Show our loading div $('div.loading').removeClass('hidden'); - if (myMatchedMetrics.length === 0) { - myMatchedMetrics = myLoadedMetrics; + // Use loaded-from-beginning metric cache by default; + // but if search results seem to exist, use those instead. + var myLocalMetrics = myLoadedMetrics; + if (myMatchedMetrics.length != 0) { + myLocalMetrics = myMatchedMetrics; } for (var target = ((myPageIndex - 1) * myMetricsPerPage); target < (myPageIndex * myMetricsPerPage); target += 1) { // Only run if we have an actual target, ignore end of page undefs - if (myMatchedMetrics[target] !== undefined) { + if (myLocalMetrics[target] !== undefined) { // unique identifier so we can track each metric to its DOM element - hash = CryptoJS.SHA256(myMatchedMetrics[target]).toString(CryptoJS.enc.Hex); + hash = CryptoJS.SHA256(myLocalMetrics[target]).toString(CryptoJS.enc.Hex); $('div.metrics').append('
'); $.ajax({ accepts: {'json': 'application/json'}, @@ -99,8 +102,8 @@ var renderSparklines = function() { dataType: 'json', error: function(xhr, textStatus, errorThrown) { console.log(errorThrown); }, hash: hash, - target: myMatchedMetrics[target], - url: graphiteUrl + '/render?target=' + myMatchedMetrics[target] + '&from=-' + myInterval + '&format=json' + target: myLocalMetrics[target], + url: graphiteUrl + '/render?target=' + myLocalMetrics[target] + '&from=-' + myInterval + '&format=json' }).done(function(output) { if (output.length === 1) { var data = []; @@ -142,7 +145,7 @@ var renderSparklines = function() { } } // Disable infinite scrolling - if (myPageIndex * myMetricsPerPage > myMatchedMetrics.length) { + if (myPageIndex * myMetricsPerPage > myLocalMetrics.length) { myPageIndex = 0; } }; @@ -250,8 +253,16 @@ var scrollNextPage = function() { if (!this.lastCall || (delta > threshold)) { this.lastCall = now; myPageIndex += 1; - console.log("loading more metrics, myPageIndex is now " + myPageIndex); - renderGraphs(); + // Actually load more metrics + loadMetricsPage(myPageIndex).done(function(d) { + if (d.length != 0) { + myLoadedMetrics = myLoadedMetrics.concat(d); + } + // Ensure the new metrics get rendered (basically just passes + // through to renderSparklines). Do this inside the callback or + // things don't line up right. + renderGraphs(); + }); } else { console.log("throttling"); }