Bug/11 ensure loaded raises on false (#18)

* Extract some logic from Section onto a module
* PreconditionNotMetError inherits from Tabasco::Error
* ensure_loaded behavior raises when proc returns falsy values
* ensure_loaded always verify the container can be found

In order to make our framework even safer to use, we want to make it harder
to bypass the verification that the container can be found in the DOM.

So even if you provide an ensure_loaded block, we always verify your container
can be found.

.rubocop.yml

@@ -370,6 +370,9 @@ RSpec/StubbedMock:
 RSpec/SubjectDeclaration:
   Enabled: true
 
+RSpec/VerifiedDoubles:
+  Enabled: false
+
 Security/IoMethods:
   Enabled: true
 

lib/tabasco.rb

@@ -4,8 +4,10 @@
 require "capybara/rspec/matchers"
 
 require_relative "tabasco/version"
-require_relative "tabasco/page"
 
 module Tabasco
   class Error < StandardError; end
+  class PreconditionNotMetError < Error; end
 end
+
+require_relative "tabasco/page"

lib/tabasco/section.rb

@@ -1,10 +1,11 @@
 # frozen_string_literal: true
 
-module Tabasco
-  PreconditionNotMetError = Class.new(StandardError)
+require_relative "section/ensure_loaded"
 
+module Tabasco
   class Section
     include Capybara::RSpecMatchers
+    include EnsureLoaded
 
     private_class_method :new # Use .load instead (or .visit for Page objects)
 
@@ -29,14 +30,6 @@ def self.test_id
       @test_id
     end
 
-    def self.ensure_loaded(&block)
-      @ensure_loaded_block = block
-    end
-
-    def self.ensure_loaded_block
-      @ensure_loaded_block
-    end
-
     # rubocop: disable Metrics/MethodLength
     def self.section(name, klass = nil, test_id: nil, &block)
       test_id = (test_id || name).to_s.tr("_", "-")
@@ -137,17 +130,6 @@ def initialize(_parent_scope: nil, **kwargs)
       raise ArgumentError, "Missing attribute(s) passed to #{self.class}: #{missing_attributes}"
     end
 
-    # Instance method to call the stored block
-    def ensure_loaded
-      unless self.class.ensure_loaded_block
-        raise "Subclasses of Tabasco::Section must define how to check whether your " \
-              "content has loaded with the ensure_loaded { ... } DSL method."
-
-      end
-
-      instance_exec(&self.class.ensure_loaded_block)
-    end
-
     def container
       unless self.class.test_id
         raise "Container not configured. Define a container with `container_test_id <test_id>` in #{self.class}."
@@ -162,14 +144,15 @@ def container
     # And narrows any node dom finding operations to the container wrapper
     (Capybara::Session::NODE_METHODS + [:within]).each do |method|
       class_eval <<~METHOD, __FILE__, __LINE__ + 1
-        def #{method}(...)                           # def find(...)
-          wrapping { Capybara.current_session.#{method}(...) }      #  wrapping { Capybara.current_session.find(...) }
-        end                                          # end
+        def #{method}(...)                                       # def find(...)
+          wrapping { Capybara.current_session.#{method}(...) }   #  wrapping { Capybara.current_session.find(...) }
+        end                                                      # end
       METHOD
     end
 
     # Allows section objects to be used as arguments of Capybara::Session#within
-    alias_method :to_capybara_node, :container
+    # Can't use alias since we need container to resolve in subclasses (for test mostly)
+    def to_capybara_node = container
 
     private
 

lib/tabasco/section/ensure_loaded.rb

@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+module Tabasco
+  class Section
+    module EnsureLoaded
+      def self.included(base)
+        base.extend(ClassMethods)
+      end
+
+      module ClassMethods
+        def ensure_loaded(&block)
+          @ensure_loaded_block = block
+        end
+
+        def ensure_loaded_block
+          @ensure_loaded_block
+        end
+      end
+
+      # Instance method to call the stored block
+      def ensure_loaded
+        unless self.class.ensure_loaded_block
+          raise "Page and section objects must define how to check whether their " \
+                "content has loaded with the ensure_loaded { ... } DSL method."
+
+        end
+
+        return if instance_exec(&self.class.ensure_loaded_block) && container
+
+        raise PreconditionNotMetError
+      end
+    end
+  end
+end

spec/fixtures/section_spec.html

@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html lang="en">
+  <head>
+    <meta charset="UTF-8" />
+    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
+    <title>Oyster Tabasco Testing Demo Page</title>
+  </head>
+  <body>
+    <header data-testid="section-container">
+      <h1>Welcome to the Testing Demo Page</h1>
+      <p>This page is for system testing purposes.</p>
+
+      <div data-testid="lorem">Lorem</div>
+      <div data-testid="ipsum">
+        Ipsum:
+        <div data-testid="dolor">Dolor</div>
+      </div>
+    </header>
+  </body>
+</html>

spec/tabasco/section/ensure_loaded_spec.rb

@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+RSpec.describe Tabasco::Section::EnsureLoaded do
+  let(:dummy_class) do
+    Class.new do
+      include Tabasco::Section::EnsureLoaded
+
+      def initialize(loaded, container = nil)
+        @loaded = loaded
+        @container = container
+      end
+
+      def loaded?
+        @loaded
+      end
+
+      attr_reader :container
+    end
+  end
+
+  describe ".ensure_loaded" do
+    it "stores the block to be used for checking if content is loaded" do
+      dummy_class.ensure_loaded { loaded? }
+      expect(dummy_class.ensure_loaded_block).to be_a(Proc)
+    end
+  end
+
+  describe "#ensure_loaded" do
+    context "when ensure_loaded block is defined" do
+      before do
+        dummy_class.ensure_loaded { loaded? }
+      end
+
+      it "does not raise an error if the block evaluates to true and the container is found" do
+        instance = dummy_class.new(true, double("dummy container"))
+        expect { instance.ensure_loaded }.not_to raise_error
+      end
+
+      it "raises PreconditionNotMetError if the block evaluates to false" do
+        instance = dummy_class.new(false, double("dummy container"))
+        expect { instance.ensure_loaded }.to raise_error(Tabasco::PreconditionNotMetError)
+      end
+
+      it "raises PreconditionNotMetError if the block evaluates to true but the container is not found" do
+        instance = dummy_class.new(false, nil)
+        expect { instance.ensure_loaded }.to raise_error(Tabasco::PreconditionNotMetError)
+      end
+    end
+
+    context "when ensure_loaded block is not defined" do
+      it "raises an error indicating the block must be defined" do
+        instance = dummy_class.new(true, double("dummy container"))
+        expect do
+          instance.ensure_loaded
+        end.to raise_error(RuntimeError, /must define how to check whether their content has loaded/)
+      end
+    end
+
+    context "when ensure_loaded block raises an error" do
+      it "propagates the error" do
+        dummy_class.ensure_loaded { raise "Some error" }
+        instance = dummy_class.new(true, double("dummy container"))
+        expect { instance.ensure_loaded }.to raise_error(RuntimeError, "Some error")
+      end
+    end
+  end
+end

spec/tabasco/section_spec.rb

@@ -3,28 +3,24 @@
 RSpec.describe Tabasco::Section do
   let(:section_klass) do
     Class.new(described_class) do
-      container_test_id "root"
+      container_test_id :section_container
 
-      attribute :user
-      attribute :customer_id
-
-      # Override so we can do unit tests without any DOM dependency
       ensure_loaded { true }
 
-      section :lorem do
-        ensure_loaded { true }
-      end
+      attribute :user
+      attribute :customer_id
 
+      section :lorem
       section :ipsum do
-        ensure_loaded { true }
-
-        section :dolor do
-          ensure_loaded { true }
-        end
+        section :dolor
       end
     end
   end
 
+  before do
+    Capybara.current_session.visit("section_spec.html")
+  end
+
   it "can access sections with dot notation" do
     section = section_klass.load(user: "John", customer_id: 123)
 
@@ -93,20 +89,22 @@
 
     it "only passes attributes to explicit classes if they define that attribute" do
       subclass = Class.new(Tabasco::Section) do
+        container_test_id :lorem
         ensure_loaded { true }
 
         attribute :user
-        section :subsub
       end
 
       section_klass = Class.new(described_class) do
+        container_test_id :section_container
         ensure_loaded { true }
 
         attribute :user
         attribute :customer
 
         section :subsection, subclass
-        section :anonymous_subsection do
+
+        section :anonymous_subsection, test_id: :ipsum do
           ensure_loaded { true }
         end
       end