diff options
author | charlie ablett <cablett@gitlab.com> | 2019-05-24 01:43:47 +0300 |
---|---|---|
committer | charlieablett <cablett@gitlab.com> | 2019-05-31 10:57:02 +0300 |
commit | 699532232ca27e6079c553261e0ab1d17317472a (patch) | |
tree | 47c7e56de28ba1857add8ea13627de07936e117e /spec | |
parent | 5f0c230a18b677bd4ec6a4a54085775b0c69a498 (diff) |
Apply reviewer feedback
- Comply doc with guidelines
- Improve tests for readability and completeness
- Separate out phases visually with newlines
- Add `format_message` test
- test readability
- code and test structure/styling
- static query analyzers
- call `as_json` on `provided_variables`
- add exception handling
Diffstat (limited to 'spec')
-rw-r--r-- | spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb | 10 | ||||
-rw-r--r-- | spec/lib/gitlab/graphql_logger_spec.rb | 13 | ||||
-rw-r--r-- | spec/requests/api/graphql/gitlab_schema_spec.rb | 31 | ||||
-rw-r--r-- | spec/requests/api/graphql_spec.rb | 26 |
4 files changed, 50 insertions, 30 deletions
diff --git a/spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb b/spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb index cbf35c1095a..66033736e01 100644 --- a/spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb +++ b/spec/lib/gitlab/graphql/query_analyzers/logger_analyzer_spec.rb @@ -5,25 +5,19 @@ require 'spec_helper' describe Gitlab::Graphql::QueryAnalyzers::LoggerAnalyzer do subject { described_class.new } - let!(:now) { Gitlab::Metrics::System.monotonic_time } - - before do - allow(Gitlab::Metrics::System).to receive(:monotonic_time).and_return(now) - end - describe '#analyze?' do context 'feature flag disabled' do before do stub_feature_flags(graphql_logging: false) end - it 'enables the analyzer' do + it 'disables the analyzer' do expect(subject.analyze?(anything)).to be_falsey end end context 'feature flag enabled by default' do - specify do + it 'enables the analyzer' do expect(subject.analyze?(anything)).to be_truthy end end diff --git a/spec/lib/gitlab/graphql_logger_spec.rb b/spec/lib/gitlab/graphql_logger_spec.rb index c8d4dac2153..4977f98b83e 100644 --- a/spec/lib/gitlab/graphql_logger_spec.rb +++ b/spec/lib/gitlab/graphql_logger_spec.rb @@ -19,15 +19,16 @@ describe Gitlab::GraphqlLogger do it 'logs a query from JSON' do analyzer_memo = { - query_string: query, - variables: {}, - complexity: 181, - depth: 0, - duration: 7 + query_string: query, + variables: {}, + complexity: 181, + depth: 0, + duration: 7 } + output = subject.format_message('INFO', now, 'test', analyzer_memo) - data = JSON.parse(output) + data = JSON.parse(output) expect(data['severity']).to eq('INFO') expect(data['time']).to eq(now.utc.iso8601(3)) expect(data['complexity']).to eq(181) diff --git a/spec/requests/api/graphql/gitlab_schema_spec.rb b/spec/requests/api/graphql/gitlab_schema_spec.rb index 510dec5edb2..1017e409f6c 100644 --- a/spec/requests/api/graphql/gitlab_schema_spec.rb +++ b/spec/requests/api/graphql/gitlab_schema_spec.rb @@ -83,31 +83,38 @@ describe 'GitlabSchema configurations' do end end + context 'when IntrospectionQuery' do + it 'is not too complex' do + query = File.read(Rails.root.join('spec/fixtures/api/graphql/introspection.graphql')) + + post_graphql(query, current_user: nil) + + expect(graphql_errors).to be_nil + end + end + context 'logging' do let(:query) { File.read(Rails.root.join('spec/fixtures/api/graphql/introspection.graphql')) } - it 'logs the query complexity' do + it 'logs the query complexity and depth' do analyzer_memo = { - query_string: query, - variables: {}, - complexity: 181, - depth: 0, - duration: "7ms" + query_string: query, + variables: {}.to_s, + complexity: 181, + depth: 0, + duration: 7 } + expect_any_instance_of(Gitlab::Graphql::QueryAnalyzers::LoggerAnalyzer).to receive(:duration).and_return(7) expect(Gitlab::GraphqlLogger).to receive(:info).with(analyzer_memo) post_graphql(query, current_user: nil) end - end - context 'when IntrospectionQuery' do - it 'is not too complex' do - query = File.read(Rails.root.join('spec/fixtures/api/graphql/introspection.graphql')) + it 'logs using `format_message`' do + expect_any_instance_of(Gitlab::GraphqlLogger).to receive(:format_message) post_graphql(query, current_user: nil) - - expect(graphql_errors).to be_nil end end end diff --git a/spec/requests/api/graphql_spec.rb b/spec/requests/api/graphql_spec.rb index abc24fc0fe8..656d6f8b50b 100644 --- a/spec/requests/api/graphql_spec.rb +++ b/spec/requests/api/graphql_spec.rb @@ -19,16 +19,21 @@ describe 'GraphQL' do context 'logging' do shared_examples 'logging a graphql query' do let(:expected_params) do - { query_string: query, variables: variables.to_json, duration: anything, depth: 1, complexity: 1 } + { query_string: query, variables: variables.to_s, duration: anything, depth: 1, complexity: 1 } end it 'logs a query with the expected params' do + expect(Gitlab::GraphqlLogger).to receive(:info).with(expected_params).once + post_graphql(query, variables: variables) end - end - before do - expect(Gitlab::GraphqlLogger).to receive(:info).with(expected_params).once + it 'does not instantiate any query analyzers' do # they are static and re-used + expect(GraphQL::Analysis::QueryComplexity).not_to receive(:new) + expect(GraphQL::Analysis::QueryDepth).not_to receive(:new) + + 2.times { post_graphql(query, variables: variables) } + end end context 'with no variables' do @@ -44,6 +49,19 @@ describe 'GraphQL' do it_behaves_like 'logging a graphql query' end + + context 'when there is an error in the logger' do + before do + allow_any_instance_of(Gitlab::Graphql::QueryAnalyzers::LoggerAnalyzer).to receive(:process_variables).and_raise(StandardError.new("oh noes!")) + end + + it 'logs the exception in Sentry and continues with the request' do + expect(Gitlab::Sentry).to receive(:track_exception).at_least(1).times + expect(Gitlab::GraphqlLogger).to receive(:info) + + post_graphql(query, variables: {}) + end + end end context 'invalid variables' do |