Skip to content

Commit

Permalink
Rewriting Queryable#query specs to test behavior
Browse files Browse the repository at this point in the history 
Specs for `Queryable#query` tested internal implementation rather than
behavior, this caused issues for classes mixing in `Queryable`, but
delegating `#query` to some collaborator. See,
e.g. ActiveTriples/ActiveTriples#169.

This tests the contract directly, instead.
  • Loading branch information
Tom Johnson committed Oct 31, 2015
1 parent 504de90 commit cf76fba
Showing 1 changed file with 110 additions and 11 deletions.
121 changes: 110 additions & 11 deletions lib/rdf/spec/queryable.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,26 +82,126 @@
end
end

it "calls #query_pattern" do

This comment has been minimized.

Sign in to view

Copy link
@gkellogg

gkellogg Oct 31, 2015

Member

This is correct, Queryable#query does indeed either call query_pattern or query_execute, and this is not intended to be overridden. (Meaning, this test should remain)

This comment has been minimized.

Sign in to view

Copy link
@no-reply

no-reply Oct 31, 2015

Member

@gkellogg: in the base implementation it does, but in implementations that use Forwardable or other delegation approaches to implement #query, that may not be the case.

In the linked ActiveTriples issue, the problem is that we have RDFSource delegating #query to #graph, but aiming to conform to the query interface. It will fail these tests because #query call's #graph's _pattern and _execute methods. We can't delegate those methods to #graph because they are protected.

What would you advise?

It seems our options are:

  • Consider #query to be the relevant interface and test its behavior.
  • Make #query_execute and #query_pattern public and consider them to be part of the interface.
  • Do def query_execute; graph.send(:query_execute); end; this feels like a nasty hack and I strongly prefer one of the first two.

Can you help clarify?

This comment has been minimized.

Sign in to view

Copy link
@gkellogg

gkellogg Oct 31, 2015

Member

Doesn't make sense to make public, as they're not intended to be used directly. As I said elsewhere, delegating #query probably makes the most sense.
is_expected.to receive(:query_pattern)
is_expected.not_to receive(:query_execute)
subject.query([:s, :p, :o]) {}
context "when called" do
it "requires an argument" do
expect { subject.query }.to raise_error(ArgumentError)
end

it "yields to the given block" do
expect { |b| subject.query(RDF::Query::Pattern.new, &b) }.to yield_control.exactly(queryable.count).times
end

it "yields statements" do
subject.query(RDF::Query::Pattern.new) do |statement|
expect(statement).to be_a_statement
end
end

context "with specific patterns" do
# Note that "01" should not match 1, per data-r2/expr-equal/sameTerm
{
[RDF::URI("http://example.org/xi1"), RDF::URI("http://example.org/p"), 1] => [RDF::Statement.from([RDF::URI("http://example.org/xi1"), RDF::URI("http://example.org/p"), 1])],
[RDF::URI("http://example.org/xi1"), RDF::URI("http://example.org/p"), nil] => [RDF::Statement.from([RDF::URI("http://example.org/xi1"), RDF::URI("http://example.org/p"), 1])],
[RDF::URI("http://example.org/xi1"), nil, 1] => [RDF::Statement.from([RDF::URI("http://example.org/xi1"), RDF::URI("http://example.org/p"), 1])],
[nil, RDF::URI("http://example.org/p"), 1] => [RDF::Statement.from([RDF::URI("http://example.org/xi1"), RDF::URI("http://example.org/p"), 1]), RDF::Statement.from([RDF::URI("http://example.org/xi2"), RDF::URI("http://example.org/p"), 1])],
[nil, nil, 1] => [RDF::Statement.from([RDF::URI("http://example.org/xi1"), RDF::URI("http://example.org/p"), 1]), RDF::Statement.from([RDF::URI("http://example.org/xi2"), RDF::URI("http://example.org/p"), 1])],
[nil, RDF::URI("http://example.org/p"), RDF::Literal::Double.new("1.0e0")] => [RDF::Statement.from([RDF::URI("http://example.org/xd1"), RDF::URI("http://example.org/p"), RDF::Literal::Double.new("1.0e0")])],
}.each do |pattern, result|
pattern = RDF::Query::Pattern.from(pattern)
it "returns #{result.inspect} given #{pattern.inspect}" do
solutions = []
subject.query(pattern) {|s| solutions << s}
expect(solutions).to eq result
end
end
end

context "with context", unless: RDF::VERSION.to_s >= "1.99" do
it "returns statements from all contexts with no context" do
pattern = RDF::Query::Pattern.new(nil, nil, nil, context: nil)
solutions = []
subject.query(pattern) { |s| solutions << s }
expect(solutions.size).to eq @statements.size
end

it "returns statements from unnamed contexts with false context" do
pattern = RDF::Query::Pattern.new(nil, nil, nil, context: false)
solutions = []
subject.query(pattern) { |s| solutions << s }
context_statements = subject.statements.reject { |st| st.has_context? }.length
expect(solutions.size).to eq context_statements
end

it "returns statements from named contexts with variable context" do
unless subject.contexts.to_a.empty?
pattern = RDF::Query::Pattern.new(nil, nil, nil, context: :c)
solutions = []
subject.query(pattern) { |s| solutions << s }
context_statements = subject.statements.select {|st| st.has_context?}.length
expect(solutions.size).to eq context_statements
end
end

it "returns statements from specific context with URI context" do
unless subject.contexts.to_a.empty?
pattern = RDF::Query::Pattern.new(nil, nil, nil, context: RDF::URI("http://ar.to/#self"))
solutions = []
subject.query(pattern) { |s| solutions << s }
expect(solutions.size).to eq File.readlines(@doap).grep(/^<http:\/\/ar.to\/\#self>/).size
end
end
end

context "with graph_name", if: RDF::VERSION.to_s >= "1.99" do
it "returns statements from all graphs with no graph_name" do
pattern = RDF::Query::Pattern.new(nil, nil, nil, graph_name: nil)
solutions = []
subject.query(pattern) { |s| solutions << s }
expect(solutions.size).to eq @statements.size
end

it "returns statements from unnamed graphss with false graph_name" do
pattern = RDF::Query::Pattern.new(nil, nil, nil, graph_name: false)
solutions = []
subject.query(pattern) { |s| solutions << s }
named_statements = subject.statements.reject {|st| st.has_name?}.length
expect(solutions.size).to eq named_statements
end

it "returns statements from named graphss with variable graph_name" do
unless subject.graph_names.to_a.empty?
pattern = RDF::Query::Pattern.new(nil, nil, nil, graph_name: :c)
solutions = []
subject.query(pattern) { |s| solutions << s }
named_statements = subject.statements.select {|st| st.has_name?}.length
expect(solutions.size).to eq named_statements
end
end

it "returns statements from specific graph with URI graph_name" do
unless subject.graph_names.to_a.empty?
pattern = RDF::Query::Pattern.new(nil, nil, nil, graph_name: RDF::URI("http://ar.to/#self"))
solutions = []
subject.query(pattern) { |s| solutions << s }
expect(solutions.size).to eq File.readlines(@doap).grep(/^<http:\/\/ar.to\/\#self>/).size
end
end
end
end
end

context "with a Query argument" do
it "yields to the given block" do
expect { |b| subject.query(query, &b) }
.to yield_control.exactly(queryable.count).times
end

it "yields statements" do
subject.query(query) do |solution|
expect(solution).not_to be_a_statement
expect(solution).to be_a RDF::Query::Solution
end
end

it "calls #query_execute" do

This comment has been minimized.

Sign in to view

Copy link
@gkellogg

gkellogg Oct 31, 2015

Member

This should stay too.
is_expected.to receive(:query_execute)
is_expected.not_to receive(:query_pattern)
subject.query(query) {}
end
end
end

Expand Down Expand Up @@ -250,7 +350,6 @@
it "defines a protected #query_pattern method" do
expect(subject.class.protected_method_defined?(:query_pattern)).to be_truthy
end

context "when called" do
it "requires an argument" do
expect { subject.send(:query_pattern) }.to raise_error(ArgumentError)
Expand Down

0 comments on commit cf76fba

Please sign in to comment.