From b7d5c154c48ed0f455323f2d0ddfb8a88ac67297 Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Wed, 21 Jan 2026 17:18:10 -0800 Subject: [PATCH 1/9] AP-527 Move healthchecks to OkComputer --- .rubocop.yml | 7 + .yarnrc | 2 +- CHANGES.md | 4 + Gemfile | 1 + Gemfile.lock | 4 + app/controllers/health_controller.rb | 19 - app/lib/health/check.rb | 149 -------- app/lib/health/result.rb | 30 -- app/lib/health/status.rb | 28 -- app/lib/health_checks.rb | 3 + app/lib/health_checks/iiif_server_check.rb | 52 +++ app/lib/health_checks/lending_root_path.rb | 31 ++ app/lib/health_checks/test_item_exists.rb | 34 ++ app/lib/lending/config.rb | 2 +- config/initializers/okcomputer.rb | 24 ++ config/routes.rb | 7 +- spec/lib/health/status_spec.rb | 28 -- .../health_checks/iiif_server_check_spec.rb | 300 +++++++++++++++ .../health_checks/lending_root_path_spec.rb | 110 ++++++ .../health_checks/test_item_exists_spec.rb | 80 ++++ spec/lib/lending/config_exception_spec.rb | 7 + spec/lib/lending/config_spec.rb | 58 +++ spec/request/health_request_spec.rb | 341 ++---------------- spec/system/health_system_spec.rb | 59 ++- 24 files changed, 771 insertions(+), 609 deletions(-) delete mode 100644 app/controllers/health_controller.rb delete mode 100644 app/lib/health/check.rb delete mode 100644 app/lib/health/result.rb delete mode 100644 app/lib/health/status.rb create mode 100644 app/lib/health_checks.rb create mode 100644 app/lib/health_checks/iiif_server_check.rb create mode 100644 app/lib/health_checks/lending_root_path.rb create mode 100644 app/lib/health_checks/test_item_exists.rb create mode 100644 config/initializers/okcomputer.rb delete mode 100644 spec/lib/health/status_spec.rb create mode 100644 spec/lib/health_checks/iiif_server_check_spec.rb create mode 100644 spec/lib/health_checks/lending_root_path_spec.rb create mode 100644 spec/lib/health_checks/test_item_exists_spec.rb create mode 100644 spec/lib/lending/config_exception_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index a05ff8c..9aa5465 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -332,3 +332,10 @@ Rails/RedundantPresenceValidationOnBelongsTo: # new in 2.13 Enabled: true Rails/RootJoinChain: # new in 2.13 Enabled: true + +############################################################ +# Fresh New Cops! +RSpec/IdenticalEqualityAssertion: # new in 2.4 + Enabled: true +RSpec/Rails/AvoidSetupHook: # new in 2.4 + Enabled: true diff --git a/.yarnrc b/.yarnrc index 19def51..3cc45ad 100644 --- a/.yarnrc +++ b/.yarnrc @@ -2,4 +2,4 @@ # yarn lockfile v1 -lastUpdateCheck 1762849565021 +lastUpdateCheck 1769038590144 diff --git a/CHANGES.md b/CHANGES.md index f91f0bb..41d9c2d 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,7 @@ +# 1.7.6 (2026-01-16) + +* Replace old health checks with OKComputer + # 1.7.5 (2025-12-17) * Hotfix for release workflow issue diff --git a/Gemfile b/Gemfile index 7c21827..0e38aa9 100644 --- a/Gemfile +++ b/Gemfile @@ -17,6 +17,7 @@ gem 'jsbundling-rails' gem 'jwt', '~> 2.2' # Workaround for https://github.com/alexspeller/non-stupid-digest-assets/issues/54 gem 'non-stupid-digest-assets', git: 'https://github.com/BerkeleyLibrary/non-stupid-digest-assets.git', ref: '1de0c38' +gem 'okcomputer', '~> 1.19', '>= 1.19.1' gem 'omniauth-cas', '~> 2.0' gem 'pagy', '~> 5.6' gem 'pg', '~> 1.2' diff --git a/Gemfile.lock b/Gemfile.lock index 9329a86..abc3ff7 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -79,6 +79,7 @@ GEM amazing_print (1.4.0) ast (2.4.2) awesome_print (1.9.2) + benchmark (0.5.0) berkeley_library-alma (0.0.7.1) berkeley_library-logging (~> 0.2) berkeley_library-marc (~> 0.3.1) @@ -217,6 +218,8 @@ GEM nokogiri (1.15.2-x86_64-linux) racc (~> 1.4) oj (3.14.3) + okcomputer (1.19.1) + benchmark omniauth (1.9.2) hashie (>= 3.4.6) rack (>= 1.6.2, < 3) @@ -411,6 +414,7 @@ DEPENDENCIES jwt (~> 2.2) listen non-stupid-digest-assets! + okcomputer (~> 1.19, >= 1.19.1) omniauth-cas (~> 2.0) pagy (~> 5.6) pg (~> 1.2) diff --git a/app/controllers/health_controller.rb b/app/controllers/health_controller.rb deleted file mode 100644 index f879d1b..0000000 --- a/app/controllers/health_controller.rb +++ /dev/null @@ -1,19 +0,0 @@ -class HealthController < ApplicationController - - # Open health check endpoint, secured by firewall - def index - render_check_result - end - - def render_check_result - respond_to do |format| - format.json { render(json: check_result, status: http_status) } - end - end - - def check_result - @check_result ||= Health::Check.new.result - end - - delegate :http_status, to: :check_result -end diff --git a/app/lib/health/check.rb b/app/lib/health/check.rb deleted file mode 100644 index 9303a2d..0000000 --- a/app/lib/health/check.rb +++ /dev/null @@ -1,149 +0,0 @@ -module Health - - # Checks on the health of critical application dependencies - # - # @see https://tools.ietf.org/id/draft-inadarei-api-health-check-01.html JSON Format - # @see https://www.consul.io/docs/agent/checks.html StatusCode based on Consul - class Check - include BerkeleyLibrary::Logging - - # ############################################################ - # Constants - - ERR_IMG_SERVER_UNREACHABLE = 'Error contacting image server'.freeze - ERR_NO_COMPLETE_ITEM = 'Unable to locate complete item'.freeze - - # ############################################################ - # Public methods - - # ############################## - # Validations - - # TODO: Implement 529 fail - VALIDATION_METHODS = %i[ - no_pending_migrations - lending_root_path - test_item_exists - iiif_server_reachable - ].freeze - - def no_pending_migrations - @no_pending_migrations ||= without_exceptions do - ActiveRecord::Migration.check_pending! - Result.pass - end - end - - def lending_root_path - @lending_root_path ||= without_exceptions do - readable = Lending::Config.lending_root_path - next Result.warn('lending root path not set') unless readable - next Result.warn("not a pathname: #{readable.inspect}") unless readable.is_a?(Pathname) - next Result.warn("not a directory: #{readable}") unless readable.directory? - next Result.warn("directory not readable: #{readable}") unless readable.readable? - - Result.pass - end - end - - def test_item_exists - @test_item_exists ||= without_exceptions do - complete_item.present? ? Result.pass : Result.warn(ERR_NO_COMPLETE_ITEM) - end - end - - def iiif_server_reachable - @iiif_server_reachable ||= without_exceptions do - next Result.warn('unable to construct test image URI') unless (test_uri = iiif_test_uri) - - response = Faraday.head(test_uri) - next Result.warn("HEAD #{iiif_test_uri} returned status #{response.status}") unless response.success? - - acao_header = response.headers['Access-Control-Allow-Origin'] - next Result.warn("HEAD #{iiif_test_uri} did not return Access-Control-Allow-Origin header") if acao_header.blank? - - Result.pass - end - end - - # ############################## - # Accessors - - def result - @result ||= run_all_checks - end - - # ############################################################ - # Private methods - - # ############################## - # Checks - - def run_all_checks - status = Health::Status::PASS - details = {}.tap do |dt| - VALIDATION_METHODS.each do |check| - check_result = send(check) - status &= check_result.status - dt[check] = check_result - end - end - Result.new(status:, details:) - end - - def without_exceptions - yield - rescue StandardError => e - logger.error(e) - msg = [e.class, e.message.to_s.strip].join(': ') - Result.warn(msg) - end - - # ############################## - # Private accessors - - def active_item - return @active_item if instance_variable_defined?(:@active_item) - - @active_item = Item.active.first - end - - def inactive_item - return @inactive_item if instance_variable_defined?(:@inactive_item) - - @inactive_item = Item.inactive.first - end - - def complete_item - return @complete_item if instance_variable_defined?(:@complete_item) - - @complete_item = active_item || inactive_item - end - - def iiif_test_uri - return @iiif_test_uri if instance_variable_defined?(:@iiif_test_uri) - - @iiif_test_uri = find_iiif_test_uri - end - - def iiif_base_uri - return @iiif_base_uri if instance_variable_defined?(:@iiif_base_uri) - - @iiif_base_uri = Lending::Config.iiif_base_uri - end - - # ############################## - # Validation prerequisites - - # TODO: could we simplify this check with a newer version of iipsrv? - # see https://github.com/ruven/iipsrv/issues/190 - def find_iiif_test_uri - return unless (base_uri = iiif_base_uri) - return unless (item = complete_item) - - iiif_directory = item.iiif_directory - BerkeleyLibrary::Util::URIs.append(base_uri, iiif_directory.first_image_url_path, 'info.json') - end - - end -end diff --git a/app/lib/health/result.rb b/app/lib/health/result.rb deleted file mode 100644 index cd44257..0000000 --- a/app/lib/health/result.rb +++ /dev/null @@ -1,30 +0,0 @@ -module Health - # Encapsulates a health check result - class Result - attr_reader :status - attr_reader :details - - def initialize(status:, details: nil) - @status = status - @details = details - end - - def as_json(*) - json = { status: status.as_json } - json[:details] = details if details - json - end - - delegate :http_status, to: :status - - class << self - def pass(details = nil) - new(status: Status::PASS, details:) - end - - def warn(details) - new(status: Status::WARN, details:) - end - end - end -end diff --git a/app/lib/health/status.rb b/app/lib/health/status.rb deleted file mode 100644 index 20206d2..0000000 --- a/app/lib/health/status.rb +++ /dev/null @@ -1,28 +0,0 @@ -require 'typesafe_enum' - -module Health - # Enumerated list of health states - class Status < TypesafeEnum::Base - new(:PASS, 200) # NOTE: states should be ordered from least to most severe - new(:WARN, 429) - new(:FAIL, 503) - - # Concatenates health states, returning the more severe state. - # @return [Status] the more severe status - def &(other) - return self unless other - - self >= other ? self : other - end - - def http_status - value - end - - # Returns the status as a string, suitable for use as a JSON value. - # @return [String] the name of the status, in lower case - def as_json(*) - key.to_s.downcase - end - end -end diff --git a/app/lib/health_checks.rb b/app/lib/health_checks.rb new file mode 100644 index 0000000..7532d6f --- /dev/null +++ b/app/lib/health_checks.rb @@ -0,0 +1,3 @@ +module HealthChecks + Dir[File.join(__dir__, 'health_checks', '*.rb')].each { |f| require f } +end diff --git a/app/lib/health_checks/iiif_server_check.rb b/app/lib/health_checks/iiif_server_check.rb new file mode 100644 index 0000000..7538475 --- /dev/null +++ b/app/lib/health_checks/iiif_server_check.rb @@ -0,0 +1,52 @@ +module HealthChecks + class IiifServerCheck < OkComputer::Check + include BerkeleyLibrary::Logging + + def check + result = validate_iiif_server + mark_message result[:message] + mark_failure if result[:warning] + rescue StandardError => e + logger.error(e) + mark_message "#{e.class}: #{e.message}" + mark_failure + end + + private + + def iiif_connection + @iiif_connection ||= Faraday.new do |f| + f.options.open_timeout = 2 + f.options.timeout = 3 + end + end + + def iiif_test_uri + base_uri = Lending::Config.iiif_base_uri + return unless base_uri + + item = Item.active.first || Item.inactive.first + return unless item + + BerkeleyLibrary::Util::URIs.append( + base_uri, + item.iiif_directory.first_image_url_path, + 'info.json' + ) + end + + # Returns a hash with :message and :warning keys + def validate_iiif_server + test_uri = iiif_test_uri + return { message: 'Unable to construct test image URI', warning: true } unless test_uri + + response = iiif_connection.head(test_uri) + return { message: "HEAD #{test_uri} returned status #{response.status}", warning: true } unless response.success? + + acao_header = response.headers['Access-Control-Allow-Origin'] + return { message: "HEAD #{test_uri} missing Access-Control-Allow-Origin header", warning: true } if acao_header.blank? + + { message: 'IIIF server reachable', warning: false } + end + end +end diff --git a/app/lib/health_checks/lending_root_path.rb b/app/lib/health_checks/lending_root_path.rb new file mode 100644 index 0000000..afab140 --- /dev/null +++ b/app/lib/health_checks/lending_root_path.rb @@ -0,0 +1,31 @@ +module HealthChecks + class LendingRootPath < OkComputer::Check + include BerkeleyLibrary::Logging + + def check + result = validate_lending_root + mark_message result[:message] + mark_failure if result[:warning] + rescue StandardError => e + logger.error(e) + mark_message "#{e.class}: #{e.message}" + mark_failure + end + + private + + def lending_root + @lending_root ||= Lending::Config.lending_root_path + end + + # Returns a hash with :message and :warning keys + def validate_lending_root + return { message: 'Lending root path not set', warning: true } unless lending_root + return { message: "Not a pathname: #{lending_root.inspect}", warning: true } unless lending_root.is_a?(Pathname) + return { message: "Not a directory: #{lending_root}", warning: true } unless lending_root.directory? + return { message: "Directory not readable: #{lending_root}", warning: true } unless lending_root.readable? + + { message: 'Lending root path exists and is readable', warning: false } + end + end +end diff --git a/app/lib/health_checks/test_item_exists.rb b/app/lib/health_checks/test_item_exists.rb new file mode 100644 index 0000000..93ed803 --- /dev/null +++ b/app/lib/health_checks/test_item_exists.rb @@ -0,0 +1,34 @@ +module HealthChecks + class TestItemExists < OkComputer::Check + include BerkeleyLibrary::Logging + + ERR_NO_COMPLETE_ITEM = 'Unable to locate complete item'.freeze + + def check + if complete_item + mark_message 'Test item lookup succeeded' + else + mark_message ERR_NO_COMPLETE_ITEM + mark_failure + end + rescue StandardError => e + mark_message "Failed to check item: #{e.message}" + mark_failure + end + + private + + def active_item + @active_item ||= Item.active.first + end + + def inactive_item + @inactive_item ||= Item.inactive.first + end + + def complete_item + @complete_item ||= active_item || inactive_item + end + + end +end diff --git a/app/lib/lending/config.rb b/app/lib/lending/config.rb index e5902f3..0664571 100644 --- a/app/lib/lending/config.rb +++ b/app/lib/lending/config.rb @@ -74,7 +74,7 @@ def lending_root_path_from(lending_root_dirname) unless File.directory?(lending_root_dirname) raise ConfigException, - "Invalid lending root: #{lending_root.inspect} is not a directory" + "Invalid lending root: #{lending_root_dirname.inspect} is not a directory" end Pathname.new(lending_root_dirname) diff --git a/config/initializers/okcomputer.rb b/config/initializers/okcomputer.rb new file mode 100644 index 0000000..e802b13 --- /dev/null +++ b/config/initializers/okcomputer.rb @@ -0,0 +1,24 @@ +# initializers/okcomputer.rb +# Health checks configuration +require Rails.root.join('app/lib/health_checks') + +OkComputer.logger = Rails.logger +OkComputer.check_in_parallel = true + +# Readiness: Database reachable +OkComputer::Registry.register 'database', OkComputer::ActiveRecordCheck.new + +# Check that DB migrations have run +OkComputer::Registry.register 'database-migrations', OkComputer::ActiveRecordMigrationsCheck.new + +# Not sure about this.... seems I need to do this in order to stub in RSPEC +unless Rails.env.test? + # Custom IIIF server check + OkComputer::Registry.register 'iiif-server', HealthChecks::IiifServerCheck.new + + # TODO: Custom Test Item Exists + OkComputer::Registry.register 'test-item-exists', HealthChecks::TestItemExists.new + + # TODO: Custom Lending Root Path + OkComputer::Registry.register 'lending-root-path', HealthChecks::LendingRootPath.new +end diff --git a/config/routes.rb b/config/routes.rb index 0c68ea7..b5d99c7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,10 +1,6 @@ Rails.application.routes.draw do root 'sessions#index' - defaults format: 'json' do - get 'health', to: 'health#index' - end - # Omniauth automatically handles requests to /auth/:provider. We need only # implement the callback. get '/login', to: 'sessions#new', as: :login @@ -48,4 +44,7 @@ defaults format: 'json' do get '/:directory/manifest', to: 'lending#manifest', as: :lending_manifest, constraints: valid_dirname end + + # Map OkComputer's /health/all.json to /health + get '/health', to: 'ok_computer/ok_computer#index', defaults: { format: :json } end diff --git a/spec/lib/health/status_spec.rb b/spec/lib/health/status_spec.rb deleted file mode 100644 index 116fe8f..0000000 --- a/spec/lib/health/status_spec.rb +++ /dev/null @@ -1,28 +0,0 @@ -require 'rails_helper' - -module Health - - describe Status do - describe '&' do - it 'handles nil' do - expect(Status::PASS & nil).to eq(Status::PASS) - end - - it 'handles self' do - expect(Status::PASS & Status::PASS).to eq(Status::PASS) - expect(Status::WARN & Status::WARN).to eq(Status::WARN) - end - - it 'respects order' do - expect(Status::WARN & Status::PASS).to eq(Status::WARN) - expect(Status::PASS & Status::WARN).to eq(Status::WARN) - end - - it 'supports &=' do - status = Status::PASS - status &= Status::WARN - expect(status).to eq(Status::WARN) - end - end - end -end diff --git a/spec/lib/health_checks/iiif_server_check_spec.rb b/spec/lib/health_checks/iiif_server_check_spec.rb new file mode 100644 index 0000000..fac96a9 --- /dev/null +++ b/spec/lib/health_checks/iiif_server_check_spec.rb @@ -0,0 +1,300 @@ +# spec/lib/health_checks/iiif_server_check_spec.rb +require 'rails_helper' + +RSpec.describe HealthChecks::IiifServerCheck do + subject(:check) { described_class.new } + + def run_check + check.run + check + end + + def stub_items(active_first:, inactive_first:) + active_relation = instance_double('ActiveRelation', first: active_first) + inactive_relation = instance_double('InactiveRelation', first: inactive_first) + + allow(Item).to receive(:active).and_return(active_relation) + allow(Item).to receive(:inactive).and_return(inactive_relation) + end + + describe '#check' do + it 'fails and sets message when the IIIF test uri cannot be constructed' do + allow(Lending::Config).to receive(:iiif_base_uri).and_return(nil) + + run_check + + expect(check.message).to eq('Unable to construct test image URI') + + if check.respond_to?(:failure?) + expect(check.failure?).to be(true) + else + expect(check.instance_variable_get(:@failure_occurred)).to be(true) + end + end + + it 'fails and sets message when the HEAD response is not successful' do + base_uri = URI('http://example.test/iiif/') + allow(Lending::Config).to receive(:iiif_base_uri).and_return(base_uri) + + iiif_dir = instance_double('IiifDirectory', first_image_url_path: 'some/path') + item = instance_double('Item', iiif_directory: iiif_dir) + stub_items(active_first: item, inactive_first: nil) + + test_uri = 'http://example.test/info.json' + allow(BerkeleyLibrary::Util::URIs).to receive(:append) + .with(base_uri, 'some/path', 'info.json') + .and_return(test_uri) + + response = instance_double('Faraday::Response', + success?: false, + status: 503, + headers: {}) + + connection = instance_double('Faraday::Connection') + allow(Faraday).to receive(:new).and_return(connection) + allow(connection).to receive(:head).with(test_uri).and_return(response) + + run_check + + expect(check.message).to match(/returned status 503/) + + if check.respond_to?(:failure?) + expect(check.failure?).to be(true) + else + expect(check.instance_variable_get(:@failure_occurred)).to be(true) + end + end + + it 'fails and sets message when Access-Control-Allow-Origin header is missing/blank' do + base_uri = URI('http://example.test/iiif/') + allow(Lending::Config).to receive(:iiif_base_uri).and_return(base_uri) + + iiif_dir = instance_double('IiifDirectory', first_image_url_path: 'some/path') + item = instance_double('Item', iiif_directory: iiif_dir) + stub_items(active_first: item, inactive_first: nil) + + test_uri = 'http://example.test/info.json' + allow(BerkeleyLibrary::Util::URIs).to receive(:append) + .with(base_uri, 'some/path', 'info.json') + .and_return(test_uri) + + response = instance_double('Faraday::Response', + success?: true, + status: 200, + headers: { 'Access-Control-Allow-Origin' => '' }) + + connection = instance_double('Faraday::Connection') + allow(Faraday).to receive(:new).and_return(connection) + allow(connection).to receive(:head).with(test_uri).and_return(response) + + run_check + + expect(check.message).to eq( + "HEAD #{test_uri} missing Access-Control-Allow-Origin header" + ) + + if check.respond_to?(:failure?) + expect(check.failure?).to be(true) + else + expect(check.instance_variable_get(:@failure_occurred)).to be(true) + end + end + + it 'does not fail when reachable and ACAO header present' do + base_uri = URI('http://example.test/iiif/') + allow(Lending::Config).to receive(:iiif_base_uri).and_return(base_uri) + + iiif_dir = instance_double('IiifDirectory', first_image_url_path: 'some/path') + item = instance_double('Item', iiif_directory: iiif_dir) + stub_items(active_first: item, inactive_first: nil) + + test_uri = 'http://example.test/info.json' + allow(BerkeleyLibrary::Util::URIs).to receive(:append) + .with(base_uri, 'some/path', 'info.json') + .and_return(test_uri) + + response = instance_double('Faraday::Response', + success?: true, + status: 200, + headers: { 'Access-Control-Allow-Origin' => '*' }) + + connection = instance_double('Faraday::Connection') + allow(Faraday).to receive(:new).and_return(connection) + allow(connection).to receive(:head).with(test_uri).and_return(response) + + run_check + + expect(check.message).to eq('IIIF server reachable') + + if check.respond_to?(:failure?) + expect(check.failure?).to be(false) + else + expect(check.instance_variable_get(:@failure_occurred)).not_to be(true) + end + end + + it 'fails and sets message when an exception is raised' do + base_uri = URI('http://example.test/iiif/') + allow(Lending::Config).to receive(:iiif_base_uri).and_return(base_uri) + + iiif_dir = instance_double('IiifDirectory', first_image_url_path: 'some/path') + item = instance_double('Item', iiif_directory: iiif_dir) + stub_items(active_first: item, inactive_first: nil) + + test_uri = 'http://example.test/info.json' + allow(BerkeleyLibrary::Util::URIs).to receive(:append) + .with(base_uri, 'some/path', 'info.json') + .and_return(test_uri) + + connection = instance_double('Faraday::Connection') + allow(Faraday).to receive(:new).and_return(connection) + allow(connection).to receive(:head).with(test_uri).and_raise(StandardError, 'boom') + + run_check + + expect(check.message).to match(/StandardError: boom/) + + if check.respond_to?(:failure?) + expect(check.failure?).to be(true) + else + expect(check.instance_variable_get(:@failure_occurred)).to be(true) + end + end + end + + describe 'private helpers' do + describe '#iiif_connection' do + it 'configures Faraday timeouts' do + conn = check.send(:iiif_connection) + + expect(conn.options.open_timeout).to eq(2) + expect(conn.options.timeout).to eq(3) + end + end + + describe '#iiif_test_uri' do + it 'returns nil when iiif_base_uri is nil' do + allow(Lending::Config).to receive(:iiif_base_uri).and_return(nil) + + expect(check.send(:iiif_test_uri)).to be_nil + end + + it 'returns nil when no Item exists' do + allow(Lending::Config).to receive(:iiif_base_uri).and_return(URI('http://example.test/iiif/')) + stub_items(active_first: nil, inactive_first: nil) + + expect(check.send(:iiif_test_uri)).to be_nil + end + + it 'builds a test uri from an active item (or inactive fallback)' do + base_uri = URI('http://example.test/iiif/') + allow(Lending::Config).to receive(:iiif_base_uri).and_return(base_uri) + + iiif_dir = instance_double('IiifDirectory', first_image_url_path: 'some/path') + item = instance_double('Item', iiif_directory: iiif_dir) + stub_items(active_first: item, inactive_first: nil) + + expect(BerkeleyLibrary::Util::URIs).to receive(:append) + .with(base_uri, 'some/path', 'info.json') + .and_return('http://example.test/iiif/some/path/info.json') + + expect(check.send(:iiif_test_uri)).to eq('http://example.test/iiif/some/path/info.json') + end + end + + describe '#validate_iiif_server' do + it 'returns a warning when it cannot construct test uri' do + allow(Lending::Config).to receive(:iiif_base_uri).and_return(nil) + + result = check.send(:validate_iiif_server) + + expect(result).to eq(message: 'Unable to construct test image URI', warning: true) + end + + it 'returns a warning when the HEAD response is not successful' do + base_uri = URI('http://example.test/iiif/') + allow(Lending::Config).to receive(:iiif_base_uri).and_return(base_uri) + + iiif_dir = instance_double('IiifDirectory', first_image_url_path: 'some/path') + item = instance_double('Item', iiif_directory: iiif_dir) + stub_items(active_first: item, inactive_first: nil) + + test_uri = 'http://example.test/info.json' + allow(BerkeleyLibrary::Util::URIs).to receive(:append) + .with(base_uri, 'some/path', 'info.json') + .and_return(test_uri) + + response = instance_double('Faraday::Response', + success?: false, + status: 503, + headers: {}) + + connection = instance_double('Faraday::Connection') + allow(Faraday).to receive(:new).and_return(connection) + allow(connection).to receive(:head).with(test_uri).and_return(response) + + result = check.send(:validate_iiif_server) + + expect(result[:warning]).to be(true) + expect(result[:message]).to match(/returned status 503/) + end + + it 'returns a warning when Access-Control-Allow-Origin header is missing/blank' do + base_uri = URI('http://example.test/iiif/') + allow(Lending::Config).to receive(:iiif_base_uri).and_return(base_uri) + + iiif_dir = instance_double('IiifDirectory', first_image_url_path: 'some/path') + item = instance_double('Item', iiif_directory: iiif_dir) + stub_items(active_first: item, inactive_first: nil) + + test_uri = 'http://example.test/info.json' + allow(BerkeleyLibrary::Util::URIs).to receive(:append) + .with(base_uri, 'some/path', 'info.json') + .and_return(test_uri) + + response = instance_double('Faraday::Response', + success?: true, + status: 200, + headers: { 'Access-Control-Allow-Origin' => '' }) + + connection = instance_double('Faraday::Connection') + allow(Faraday).to receive(:new).and_return(connection) + allow(connection).to receive(:head).with(test_uri).and_return(response) + + result = check.send(:validate_iiif_server) + + expect(result).to eq( + message: "HEAD #{test_uri} missing Access-Control-Allow-Origin header", + warning: true + ) + end + + it 'returns ok when reachable and ACAO header present' do + base_uri = URI('http://example.test/iiif/') + allow(Lending::Config).to receive(:iiif_base_uri).and_return(base_uri) + + iiif_dir = instance_double('IiifDirectory', first_image_url_path: 'some/path') + item = instance_double('Item', iiif_directory: iiif_dir) + stub_items(active_first: item, inactive_first: nil) + + test_uri = 'http://example.test/info.json' + allow(BerkeleyLibrary::Util::URIs).to receive(:append) + .with(base_uri, 'some/path', 'info.json') + .and_return(test_uri) + + response = instance_double('Faraday::Response', + success?: true, + status: 200, + headers: { 'Access-Control-Allow-Origin' => '*' }) + + connection = instance_double('Faraday::Connection') + allow(Faraday).to receive(:new).and_return(connection) + allow(connection).to receive(:head).with(test_uri).and_return(response) + + result = check.send(:validate_iiif_server) + + expect(result).to eq(message: 'IIIF server reachable', warning: false) + end + end + end +end diff --git a/spec/lib/health_checks/lending_root_path_spec.rb b/spec/lib/health_checks/lending_root_path_spec.rb new file mode 100644 index 0000000..24c8bf7 --- /dev/null +++ b/spec/lib/health_checks/lending_root_path_spec.rb @@ -0,0 +1,110 @@ +# spec/lib/health_checks/lending_root_path_spec.rb +require 'rails_helper' + +RSpec.describe HealthChecks::LendingRootPath do + subject(:check) { described_class.new } + + def run_check + check.run + check + end + + def expect_failed + if check.respond_to?(:failure?) + expect(check.failure?).to be(true) + else + expect(check.instance_variable_get(:@failure_occurred)).to be(true) + end + end + + def expect_not_failed + if check.respond_to?(:failure?) + expect(check.failure?).to be(false) + else + expect(check.instance_variable_get(:@failure_occurred)).not_to be(true) + end + end + + describe '#check' do + it 'fails when lending root path is not set' do + allow(Lending::Config).to receive(:lending_root_path).and_return(nil) + + run_check + + expect(check.message).to eq('Lending root path not set') + expect_failed + end + + it 'fails when lending root is not a directory' do + pn = Pathname.new('/tmp/lending-root') + allow(pn).to receive(:directory?).and_return(false) + + allow(Lending::Config).to receive(:lending_root_path).and_return(pn) + + run_check + + expect(check.message).to eq("Not a directory: #{pn}") + expect_failed + end + + it 'fails when directory is not readable' do + pn = Pathname.new('/tmp/lending-root') + allow(pn).to receive(:directory?).and_return(true) + allow(pn).to receive(:readable?).and_return(false) + + allow(Lending::Config).to receive(:lending_root_path).and_return(pn) + + run_check + + expect(check.message).to eq("Directory not readable: #{pn}") + expect_failed + end + + it 'does not fail when directory exists and is readable' do + pn = Pathname.new('/tmp/lending-root') + allow(pn).to receive(:directory?).and_return(true) + allow(pn).to receive(:readable?).and_return(true) + + allow(Lending::Config).to receive(:lending_root_path).and_return(pn) + + run_check + + expect(check.message).to eq('Lending root path exists and is readable') + expect_not_failed + end + + it 'fails and sets message when an exception is raised' do + allow(Lending::Config).to receive(:lending_root_path).and_raise(StandardError, 'boom') + + run_check + + expect(check.message).to match(/StandardError: boom/) + expect_failed + end + end + + describe '#lending_root' do + it 'memoizes Lending::Config.lending_root_path' do + pn = Pathname.new('/tmp') + allow(Lending::Config).to receive(:lending_root_path).and_return(pn) + + first = check.send(:lending_root) + second = check.send(:lending_root) + + expect(first).to eq(pn) + expect(second).to eq(pn) + expect(Lending::Config).to have_received(:lending_root_path).once + end + end + + describe '#validate_lending_root' do + it 'returns a warning when lending root is not a Pathname' do + allow(Lending::Config).to receive(:lending_root_path).and_return('/tmp/not_a_pathname') + + result = check.send(:validate_lending_root) + + expect(result[:warning]).to be(true) + expect(result[:message]).to match(/Not a pathname/) + end + end +end diff --git a/spec/lib/health_checks/test_item_exists_spec.rb b/spec/lib/health_checks/test_item_exists_spec.rb new file mode 100644 index 0000000..9a92b88 --- /dev/null +++ b/spec/lib/health_checks/test_item_exists_spec.rb @@ -0,0 +1,80 @@ +require 'rails_helper' + +RSpec.describe HealthChecks::TestItemExists do + subject(:check) { described_class.new } + + def run_check + check.run + check + end + + def expect_failed + if check.respond_to?(:failure?) + expect(check.failure?).to be(true) + else + expect(check.instance_variable_get(:@failure_occurred)).to be(true) + end + end + + describe '#check' do + it 'marks success when an active item exists' do + active_relation = double('ActiveRelation') + inactive_relation = double('InactiveRelation') + item = instance_double(Item) + + allow(Item).to receive(:active).and_return(active_relation) + allow(active_relation).to receive(:first).and_return(item) + + allow(Item).to receive(:inactive).and_return(inactive_relation) + allow(inactive_relation).to receive(:first).and_return(nil) + + run_check + + expect(check.message).to eq('Test item lookup succeeded') + end + + it 'fails when no complete item exists (active and inactive are nil)' do + active_relation = double('ActiveRelation') + inactive_relation = double('InactiveRelation') + + allow(Item).to receive(:active).and_return(active_relation) + allow(active_relation).to receive(:first).and_return(nil) + + allow(Item).to receive(:inactive).and_return(inactive_relation) + allow(inactive_relation).to receive(:first).and_return(nil) + + run_check + + expect(check.message).to eq(described_class::ERR_NO_COMPLETE_ITEM) + expect_failed + end + + it 'falls back to inactive item when no active item exists' do + active_relation = double('ActiveRelation') + inactive_relation = double('InactiveRelation') + item = instance_double(Item) + + allow(Item).to receive(:active).and_return(active_relation) + allow(active_relation).to receive(:first).and_return(nil) + + allow(Item).to receive(:inactive).and_return(inactive_relation) + allow(inactive_relation).to receive(:first).and_return(item) + + run_check + + expect(check.message).to eq('Test item lookup succeeded') + end + + it 'marks failure and message when an exception occurs' do + active_relation = double('ActiveRelation') + + allow(Item).to receive(:active).and_return(active_relation) + allow(active_relation).to receive(:first).and_raise(StandardError, 'boom') + + run_check + + expect(check.message).to eq('Failed to check item: boom') + expect_failed + end + end +end diff --git a/spec/lib/lending/config_exception_spec.rb b/spec/lib/lending/config_exception_spec.rb new file mode 100644 index 0000000..178b3bd --- /dev/null +++ b/spec/lib/lending/config_exception_spec.rb @@ -0,0 +1,7 @@ +require 'rails_helper' + +RSpec.describe Lending::ConfigException do + it 'is a StandardError subclass' do + expect(described_class).to be < StandardError + end +end diff --git a/spec/lib/lending/config_spec.rb b/spec/lib/lending/config_spec.rb index b98d2d6..ad3a048 100644 --- a/spec/lib/lending/config_spec.rb +++ b/spec/lib/lending/config_spec.rb @@ -55,5 +55,63 @@ module Lending expect(Config.lending_root_path).to eq(expected_value) end end + + describe 'error handling and rails config fallbacks' do + before do + Config.instance_variable_set(:@iiif_base_uri, nil) + Config.instance_variable_set(:@lending_root_path, nil) + end + + it 'raises ConfigException when ENV IIIF base URL is invalid (covers line 70)' do + ENV[Config::ENV_IIIF_BASE] = 'http://exa mple.org/bad uri' + + expect { Config.iiif_base_uri }.to raise_error(Lending::ConfigException, /Invalid IIIF base URI:/) + end + + it 'raises ConfigException when ENV lending root is not a directory (covers lines 76-79 / 77)' do + ENV[Config::ENV_ROOT] = '/definitely/not/a/real/path' + + expect { Config.lending_root_path }.to raise_error(Lending::ConfigException, /Invalid lending root:/) + end + + it 'reads IIIF base from Rails config when ENV is unset (covers rails_iiif_base / line 52 and rails_config_value / 85-88)' do + ENV[Config::ENV_IIIF_BASE] = nil + Config.instance_variable_set(:@iiif_base_uri, nil) + + expected = URI.parse(Rails.application.config.iiif_base_url.to_s) + expect(Config.iiif_base_uri).to eq(expected) + end + + it 'reads lending root from Rails config when ENV is unset (covers rails_lending_root / lines 61-62 and rails_config_value / 85-88)' do + ENV[Config::ENV_ROOT] = nil + Config.instance_variable_set(:@lending_root_path, nil) + + expected = Pathname.new(Rails.application.config.lending_root.to_s) + expect(Config.lending_root_path).to eq(expected) + end + + it 'returns nil from rails_config_value when Rails is not defined (covers rails_config / 92-95)' do + ENV[Config::ENV_IIIF_BASE] = nil + Config.instance_variable_set(:@iiif_base_uri, nil) + + # Temporarily hide Rails constant so `defined?(Rails)` is false + rails_const = Object.const_get(:Rails) + Object.send(:remove_const, :Rails) + begin + expect { Config.iiif_base_uri }.to raise_error(Lending::ConfigException, /IIIF base URL not set/) + ensure + Object.const_set(:Rails, rails_const) + end + end + + it 'returns nil from rails_config when Rails.application is nil (covers rails_config / 92-95)' do + ENV[Config::ENV_IIIF_BASE] = nil + Config.instance_variable_set(:@iiif_base_uri, nil) + + allow(Rails).to receive(:application).and_return(nil) + + expect { Config.iiif_base_uri }.to raise_error(Lending::ConfigException, /IIIF base URL not set/) + end + end end end diff --git a/spec/request/health_request_spec.rb b/spec/request/health_request_spec.rb index 1a93f2f..6a00712 100644 --- a/spec/request/health_request_spec.rb +++ b/spec/request/health_request_spec.rb @@ -1,338 +1,45 @@ require 'rails_helper' -context HealthController, type: :request do - let(:iiif_url) { 'http://iipsrv.test/iiif/' } - let(:config_instance_vars) { %i[@iiif_base_uri @lending_root_path] } +RSpec.describe 'Health Checks', type: :request do + describe 'GET /health' do + let(:health_path) { '/health' } - RSpec::Matchers.define :be_a_health_result do - match do |response| - json_result = JSON.parse(response.body) - %w[status details].each { |k| json_result.key?(k) } - rescue JSON::ParserError - false - end - - failure_message do |response| - "expected a JSON health result, got #{response.body}" - end - end - - RSpec::Matchers.define :be_passing do - expected_status = Health::Status::PASS - - match do |response| - next false unless response.status == expected_status.http_status - next false unless (json_result = parse_result(response)) - - json_result['status'] == expected_status.as_json - end - - failure_message do |response| - if (json_result = parse_result(response)) && (details = json_result['details']) - failed_checks = details.filter_map do |check, result| - next check unless result['status'] == expected_status.as_json - end - - return "expected #{expected_status}, got #{response.status}; failed checks: #{failed_checks.join(', ')}; body: #{response.body}" - end - - "expected #{expected_status}, got #{response.status}; body: #{response.body}" - end - end - - # TODO: Implement 529 fail - RSpec::Matchers.define :be_warning do - expected_status = Health::Status::WARN - - match do |response| - next false unless response.status == expected_status.http_status - next false unless (json_result = parse_result(response)) - - json_result['status'] == expected_status.as_json - end - - failure_message do |response| - "expected #{expected_status}, got #{response.status}; body: #{response.body}" - end - end - - RSpec::Matchers.define :have_states do |expected_states_by_check| - match do |response| - next false unless (json_result = parse_result(response)) - next false unless (details = json_result['details']) - - expected_states_by_check.all? do |check, expected_state| - actual_state = (check_result = details[check.to_s]) && check_result['status'] - actual_state == expected_state.as_json - end - end - - failure_message do |response| - expected_states_msg = expected_states_by_check - .map { |check, expected_state| "#{check}: #{expected_state.as_json}" }.join(', ') - - if (json_result = parse_result(response)) && (details = json_result['details']) - mismatched_check_msg = details.each_with_object([]) do |(check, result), msg_segments| - expected_state = expected_states_by_check[check.to_sym] - actual_state = result['status'] - msg_segments << [check, actual_state].join(': ') if actual_state != expected_state.as_json - end.join(', ') - - return "expected #{expected_states_msg}, got #{mismatched_check_msg}; body: #{response.body}" - end - - "expected #{expected_states_msg}; got #{response.body}" - end - end - - def parse_result(response) - json_result = JSON.parse(response.body) - return unless json_result.is_a?(Hash) - - json_result.with_indifferent_access - rescue JSON::ParserError - nil - end - - def stub_iiif_success! - stub_request(:head, /#{iiif_url}/).to_return( - status: 200, - headers: { - 'Access-Control-Allow-Origin' => '*', - 'Access-Control-Allow-Headers' => 'X-Requested-With' - } - ) - end - - def all_passing - Health::Check::VALIDATION_METHODS.each_with_object({}) do |check, states| - states[check] = Health::Status::PASS - end - end - - def passing_except(*warn) - {}.tap do |states| - warn_checks = Array(warn) - warn_checks.each { |check| states[check] = Health::Status::WARN } - Health::Check::VALIDATION_METHODS.each do |check| - next if warn_checks.include?(check) - - states[check] = Health::Status::PASS - end - end - end - - before do - @env_orig = Lending::Config::ENV_VARS.each_with_object({}) do |var, env| - env[var] = ENV[var] - end - - @config_ivars_orig = config_instance_vars.each_with_object({}) do |var, ivals| - ivals[var] = Lending::Config.instance_variable_get(var) - end - end - - after do - @env_orig.each { |var, val| ENV[var] = val } - @config_ivars_orig.each { |var, val| Lending::Config.instance_variable_set(var, val) } - end - - describe :health do - before do - Lending::Config.instance_variable_set(:@iiif_base_uri, URI.parse('http://iipsrv.test/iiif/')) - Lending::Config.instance_variable_set(:@lending_root_path, Pathname.new('spec/data/lending')) - end - - describe 'success' do - before do - - stub_iiif_success! - create(:complete_item) - end - - it 'returns a PASS response' do - get health_path - - expect(response).to be_a_health_result - expect(response).to be_passing - - expect(response).to have_states(all_passing) - end - end - - describe 'pending migrations' do - before do - stub_iiif_success! - create(:complete_item) - - allow(ActiveRecord::Migration).to receive(:check_pending!).and_raise(ActiveRecord::PendingMigrationError) - end - - it 'returns a WARN response' do - get health_path - - expect(response).to be_a_health_result - expect(response).to be_warning - expect(response).to have_states(passing_except(:no_pending_migrations)) - end - end - - context 'IIIF server not reachable' do - before do - stub_request(:any, /#{iiif_url}/).to_raise(Errno::ECONNREFUSED) - - create(:complete_item) - end - - it 'returns a WARN response' do + context 'when all systems are functional' do + it 'returns 200 OK and success in JSON' do get health_path + expect(response).to have_http_status(:ok) - expect(response).to be_a_health_result - expect(response).to be_warning - expect(response).to have_states(passing_except(:iiif_server_reachable)) - expect(response.body).to match(/Connection refused/) + json = JSON.parse(response.body) + expect(json.dig('database', 'success')).to be true end end - context 'IIIF test image not found' do - let(:expected_status) { 404 } - + context 'when a critical service is down' do before do - stub_request(:any, /#{iiif_url}/).to_return(status: expected_status) - - create(:complete_item) - end - - it 'returns a WARN response' do - get health_path - - expect(response).to be_a_health_result - expect(response).to be_warning - expect(response).to have_states(passing_except(:iiif_server_reachable)) - expect(response.body).to include(expected_status.to_s) - end - end - - context 'IIIF server bad hostname' do - let(:expected_msg) { 'Failed to open TCP connection to test.test:80 (getaddrinfo: nodename nor servname provided, or not known)' } - - before do - stub_request(:any, /#{iiif_url}/).to_raise(SocketError.new(expected_msg)) - - create(:complete_item) - end - - it 'returns a WARN response' do - get health_path - - expect(response).to be_a_health_result - expect(response).to be_warning - expect(response).to have_states(passing_except(:iiif_server_reachable)) - expect(response.body).to include(expected_msg) - end - end - - context 'IIIF base URL not configured' do - before do - ENV[Lending::Config::ENV_IIIF_BASE] = nil - allow(Rails.application.config).to receive(Lending::Config::CONFIG_KEY_IIIF_BASE).and_return(nil) - Lending::Config.instance_variable_set(:@iiif_base_uri, nil) - - stub_iiif_success! - create(:complete_item) - end - - it 'returns a WARN response' do - get health_path - - expect(response).to be_a_health_result - expect(response).to be_warning - expect(response).to have_states(passing_except(:iiif_server_reachable)) - end - end - - context 'Invalid IIIF base URL' do - before do - ENV[Lending::Config::ENV_IIIF_BASE] = 'I am not a URI' - Lending::Config.instance_variable_set(:@iiif_base_uri, nil) - - stub_iiif_success! - create(:complete_item) - end - - it 'returns a WARN response' do - get health_path - - expect(response).to be_a_health_result - expect(response).to be_warning - expect(response).to have_states(passing_except(:iiif_server_reachable)) - end - end - - context 'no test item' do - it 'returns a WARN response' do - get health_path - - expect(response).to be_a_health_result - expect(response).to be_warning - expect(response).to have_states(passing_except(:iiif_server_reachable, :test_item_exists)) - end - end - - context 'Lending root not readable' do - before do - Dir.mktmpdir(File.basename(__FILE__, '.rb')) do |dir| - Lending::Config.instance_variable_set(:@lending_root_path, Pathname.new(dir)) + # Apparently OkComputer wraps 'check' inside 'run'. + # By stubbing 'run' on any ActiveRecordCheck, we take total control. + allow_any_instance_of(OkComputer::ActiveRecordCheck).to receive(:run) do |instance| + # Manually set the internal state of the check to 'failed' + instance.instance_variable_set(:@failure_occurred, true) + instance.instance_variable_set(:@message, 'DB Connection Error') + instance end - expect(Lending::Config.lending_root_path.directory?).to eq(false) # just to be sure - stub_iiif_success! + # Ensure the collection sees a failure: + allow_any_instance_of(OkComputer::CheckCollection).to receive(:success?).and_return(false) end - it 'returns a WARN response' do + it 'returns a 500 Internal Server Error' do get health_path - - expect(response).to be_a_health_result - expect(response).to be_warning - expect(response).to have_states(passing_except(:lending_root_path, :test_item_exists, :iiif_server_reachable)) - end - end - - context 'Lending root not configured' do - before do - ENV[Lending::Config::ENV_ROOT] = nil - allow(Rails.application.config).to receive(Lending::Config::CONFIG_KEY_ROOT).and_return(nil) - Lending::Config.instance_variable_set(:@lending_root_path, nil) - - stub_iiif_success! - end - - it 'returns a WARN response' do - get health_path - - expect(response).to be_a_health_result - expect(response).to be_warning - expect(response).to have_states(passing_except(:lending_root_path, :test_item_exists, :iiif_server_reachable)) - end - end - - context 'Invalid lending root' do - before do - Dir.mktmpdir(File.basename(__FILE__, '.rb')) do |dir| - ENV[Lending::Config::ENV_ROOT] = dir - end - expect(File.directory?(ENV[Lending::Config::ENV_ROOT])).to eq(false) # just to be sure - Lending::Config.instance_variable_set(:@lending_root_path, nil) - - stub_iiif_success! + expect(response).to have_http_status(:internal_server_error) end - it 'returns a WARN response' do + it 'reports the failure in the JSON body' do get health_path + json = JSON.parse(response.body) - expect(response).to be_a_health_result - expect(response).to be_warning - expect(response).to have_states(passing_except(:lending_root_path, :test_item_exists, :iiif_server_reachable)) + expect(json.dig('database', 'success')).to be false + expect(json.dig('database', 'message')).to eq('DB Connection Error') end end end diff --git a/spec/system/health_system_spec.rb b/spec/system/health_system_spec.rb index 91f2c07..e182b99 100644 --- a/spec/system/health_system_spec.rb +++ b/spec/system/health_system_spec.rb @@ -1,44 +1,39 @@ -require 'capybara_helper' +require 'rails_helper' -describe HealthController, type: :system do - let(:config_instance_vars) { %i[@iiif_base_uri @lending_root_path] } +RSpec.describe 'Health Check', type: :system do + describe 'Checking system health' do + it 'renders the health status as JSON when healthy' do + visit '/health' + json = JSON.parse(page.text) - before do - @config_ivars_orig = config_instance_vars.each_with_object({}) do |var, ivals| - ivals[var] = Lending::Config.instance_variable_get(var) + expect(json.dig('database', 'success')).to be true end - @webmock_config = %i[allow_localhost allow net_http_connect_on_start].each_with_object({}) do |attr, opts| - opts[attr] = WebMock::Config.instance.send(attr) - end - webmock_tmp_config = @webmock_config.dup.tap do |conf| - conf[:allow] = (conf[:allow] || []) + ['iipsrv.test'] - end - WebMock.disable_net_connect!(webmock_tmp_config) - - Lending::Config.instance_variable_set(:@iiif_base_uri, URI.parse('http://iipsrv.test/iiif/')) - Lending::Config.instance_variable_set(:@lending_root_path, Pathname.new('spec/data/lending')) + it 'returns a failure status when a critical service is down' do + # 1. Inject an environment variable into the current process. + # Note: For this to work in Docker, the Puma server must be + # running in the same container or share the ENV. + # A more robust way for Docker is to stub the ActiveRecord call itself: - create(:complete_item) - end - - after do - @config_ivars_orig.each { |var, val| Lending::Config.instance_variable_set(var, val) } + allow(ActiveRecord::Base).to receive(:connected?).and_return(false) + # Wait! The above also only works in the test process. - WebMock.disable_net_connect!(@webmock_config) - end + # INSTEAD: Let's use the Registry to register a temporary + # failing check that doesn't rely on mocks. - describe :health do - it 'returns a successful health check' do - visit health_path + begin + # Register a check that always fails + OkComputer::Registry.register 'failing-check', OkComputer::HttpCheck.new('http://localhost:9999/nonexistent') - body_expected = { - 'status' => 'pass', - 'details' => Health::Check::VALIDATION_METHODS.each_with_object({}) { |m, d| d[m.to_s] = { 'status' => 'pass' } } - } + visit '/health' - body_actual = JSON.parse(page.text) - expect(body_actual).to eq(body_expected) + json = JSON.parse(page.text) + # Verify that at least one check failed + expect(json.values.any? { |v| v['success'] == false }).to be true + ensure + # Always unregister to avoid bleeding into other tests + OkComputer::Registry.deregister 'failing-check' + end end end end From 0f0f4cd2c4aa319e1388609618c1408bd9a72350 Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Thu, 22 Jan 2026 13:06:44 -0800 Subject: [PATCH 2/9] Renaming class to fit with zeitwerk --- app/lib/health_checks/iiif_server_check.rb | 2 +- config/initializers/okcomputer.rb | 2 +- docker-compose.yml | 2 -- spec/lib/health_checks/iiif_server_check_spec.rb | 2 +- 4 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/lib/health_checks/iiif_server_check.rb b/app/lib/health_checks/iiif_server_check.rb index 7538475..6ae9e8b 100644 --- a/app/lib/health_checks/iiif_server_check.rb +++ b/app/lib/health_checks/iiif_server_check.rb @@ -1,5 +1,5 @@ module HealthChecks - class IiifServerCheck < OkComputer::Check + class IIIFServerCheck < OkComputer::Check include BerkeleyLibrary::Logging def check diff --git a/config/initializers/okcomputer.rb b/config/initializers/okcomputer.rb index e802b13..4a52c1b 100644 --- a/config/initializers/okcomputer.rb +++ b/config/initializers/okcomputer.rb @@ -14,7 +14,7 @@ # Not sure about this.... seems I need to do this in order to stub in RSPEC unless Rails.env.test? # Custom IIIF server check - OkComputer::Registry.register 'iiif-server', HealthChecks::IiifServerCheck.new + OkComputer::Registry.register 'iiif-server', HealthChecks::IIIFServerCheck.new # TODO: Custom Test Item Exists OkComputer::Registry.register 'test-item-exists', HealthChecks::TestItemExists.new diff --git a/docker-compose.yml b/docker-compose.yml index 899c773..54f8d0a 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -114,5 +114,3 @@ services: volumes: postgres_data: { } - -version: '3.8' diff --git a/spec/lib/health_checks/iiif_server_check_spec.rb b/spec/lib/health_checks/iiif_server_check_spec.rb index fac96a9..4f61156 100644 --- a/spec/lib/health_checks/iiif_server_check_spec.rb +++ b/spec/lib/health_checks/iiif_server_check_spec.rb @@ -1,7 +1,7 @@ # spec/lib/health_checks/iiif_server_check_spec.rb require 'rails_helper' -RSpec.describe HealthChecks::IiifServerCheck do +RSpec.describe HealthChecks::IIIFServerCheck do subject(:check) { described_class.new } def run_check From 78c9593900381ebbc41472c783621279e2548d89 Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Thu, 22 Jan 2026 13:55:52 -0800 Subject: [PATCH 3/9] Tweak collector spec having ordering issues --- .rubocop.yml | 7 ------- spec/lib/lending/collector_spec.rb | 10 ++-------- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 9aa5465..a05ff8c 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -332,10 +332,3 @@ Rails/RedundantPresenceValidationOnBelongsTo: # new in 2.13 Enabled: true Rails/RootJoinChain: # new in 2.13 Enabled: true - -############################################################ -# Fresh New Cops! -RSpec/IdenticalEqualityAssertion: # new in 2.4 - Enabled: true -RSpec/Rails/AvoidSetupHook: # new in 2.4 - Enabled: true diff --git a/spec/lib/lending/collector_spec.rb b/spec/lib/lending/collector_spec.rb index 9023f73..b8ab781 100644 --- a/spec/lib/lending/collector_spec.rb +++ b/spec/lib/lending/collector_spec.rb @@ -90,16 +90,10 @@ def expect_to_process(item_dirname) final_dirs = [] logger = BerkeleyLibrary::Logging.logger - - # Collect all logs in an array logs = [] - # Allow all other info calls to pass through unchanged - allow(logger).to receive(:info).and_call_original - - # Capture only the two specific log lines we assert on - allow(logger).to receive(:info).with(/starting|nothing left to process/) do |msg| - logs << msg + allow(logger).to receive(:info) do |msg| + logs << msg if msg.match?(/starting|nothing left to process/) end %w[b12345678_c12345678 b86753090_c86753090].each do |item_dir| From 9e6262a918921c43c6f485fbd3e41c3e3e8b25de Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Fri, 23 Jan 2026 16:25:16 -0800 Subject: [PATCH 4/9] Fixed request spec --- .yarnrc | 2 +- config/initializers/okcomputer.rb | 18 +++++++----------- spec/request/health_request_spec.rb | 23 ++++++++++++++++++++++- 3 files changed, 30 insertions(+), 13 deletions(-) diff --git a/.yarnrc b/.yarnrc index 3cc45ad..bf53ea5 100644 --- a/.yarnrc +++ b/.yarnrc @@ -2,4 +2,4 @@ # yarn lockfile v1 -lastUpdateCheck 1769038590144 +lastUpdateCheck 1769202471534 diff --git a/config/initializers/okcomputer.rb b/config/initializers/okcomputer.rb index 4a52c1b..366d8ff 100644 --- a/config/initializers/okcomputer.rb +++ b/config/initializers/okcomputer.rb @@ -1,6 +1,5 @@ # initializers/okcomputer.rb -# Health checks configuration -require Rails.root.join('app/lib/health_checks') +require 'health_checks' OkComputer.logger = Rails.logger OkComputer.check_in_parallel = true @@ -11,14 +10,11 @@ # Check that DB migrations have run OkComputer::Registry.register 'database-migrations', OkComputer::ActiveRecordMigrationsCheck.new -# Not sure about this.... seems I need to do this in order to stub in RSPEC -unless Rails.env.test? - # Custom IIIF server check - OkComputer::Registry.register 'iiif-server', HealthChecks::IIIFServerCheck.new +# Custom IIIF server check +OkComputer::Registry.register 'iiif-server', HealthChecks::IIIFServerCheck.new - # TODO: Custom Test Item Exists - OkComputer::Registry.register 'test-item-exists', HealthChecks::TestItemExists.new +# TODO: Custom Test Item Exists +OkComputer::Registry.register 'test-item-exists', HealthChecks::TestItemExists.new - # TODO: Custom Lending Root Path - OkComputer::Registry.register 'lending-root-path', HealthChecks::LendingRootPath.new -end +# TODO: Custom Lending Root Path +OkComputer::Registry.register 'lending-root-path', HealthChecks::LendingRootPath.new diff --git a/spec/request/health_request_spec.rb b/spec/request/health_request_spec.rb index 6a00712..6336cf8 100644 --- a/spec/request/health_request_spec.rb +++ b/spec/request/health_request_spec.rb @@ -5,12 +5,31 @@ let(:health_path) { '/health' } context 'when all systems are functional' do + before do + iiif = OkComputer::Registry.fetch('iiif-server') + test_item = OkComputer::Registry.fetch('test-item-exists') + + allow(iiif).to receive(:run) do + iiif.instance_variable_set(:@success, true) + iiif.instance_variable_set(:@message, 'OK') + iiif + end + + allow(test_item).to receive(:run) do + test_item.instance_variable_set(:@success, true) + test_item.instance_variable_set(:@message, 'OK') + test_item + end + end + it 'returns 200 OK and success in JSON' do get health_path expect(response).to have_http_status(:ok) json = JSON.parse(response.body) expect(json.dig('database', 'success')).to be true + expect(json.dig('iiif-server', 'success')).to be true + expect(json.dig('test-item-exists', 'success')).to be true end end @@ -26,7 +45,9 @@ end # Ensure the collection sees a failure: - allow_any_instance_of(OkComputer::CheckCollection).to receive(:success?).and_return(false) + allow_any_instance_of(OkComputer::CheckCollection) + .to receive(:success?) + .and_return(false) end it 'returns a 500 Internal Server Error' do From 348e95dbe0954a019acc9332e024dfeea76af56b Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Wed, 28 Jan 2026 12:26:15 -0800 Subject: [PATCH 5/9] a number of improvements based on code review feedback(logging, key names, error messages, etc) --- .yarnrc | 2 +- CHANGES.md | 4 --- app/lib/health_checks.rb | 12 ++++++++- app/lib/health_checks/iiif_server_check.rb | 14 +++++----- app/lib/health_checks/lending_root_path.rb | 16 +++++------ app/lib/health_checks/test_item_exists.rb | 7 +++-- .../health_checks/iiif_server_check_spec.rb | 16 +++++------ .../health_checks/lending_root_path_spec.rb | 6 ++--- .../health_checks/test_item_exists_spec.rb | 4 +-- spec/lib/lending/config_spec.rb | 12 ++++----- spec/system/health_system_spec.rb | 27 +++++++------------ 11 files changed, 59 insertions(+), 61 deletions(-) diff --git a/.yarnrc b/.yarnrc index bf53ea5..cd514f7 100644 --- a/.yarnrc +++ b/.yarnrc @@ -2,4 +2,4 @@ # yarn lockfile v1 -lastUpdateCheck 1769202471534 +lastUpdateCheck 1769625849915 diff --git a/CHANGES.md b/CHANGES.md index 41d9c2d..f91f0bb 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,7 +1,3 @@ -# 1.7.6 (2026-01-16) - -* Replace old health checks with OKComputer - # 1.7.5 (2025-12-17) * Hotfix for release workflow issue diff --git a/app/lib/health_checks.rb b/app/lib/health_checks.rb index 7532d6f..8cf7be2 100644 --- a/app/lib/health_checks.rb +++ b/app/lib/health_checks.rb @@ -1,3 +1,13 @@ +# frozen_string_literal: true + module HealthChecks - Dir[File.join(__dir__, 'health_checks', '*.rb')].each { |f| require f } + CHECK_FILES = %w[ + iiif_server_check + lending_root_path + test_item_exists + ].freeze + + CHECK_FILES.each do |name| + require File.join(__dir__, "health_checks/#{name}") + end end diff --git a/app/lib/health_checks/iiif_server_check.rb b/app/lib/health_checks/iiif_server_check.rb index 6ae9e8b..d4fcb03 100644 --- a/app/lib/health_checks/iiif_server_check.rb +++ b/app/lib/health_checks/iiif_server_check.rb @@ -5,10 +5,10 @@ class IIIFServerCheck < OkComputer::Check def check result = validate_iiif_server mark_message result[:message] - mark_failure if result[:warning] + mark_failure if result[:failure] rescue StandardError => e logger.error(e) - mark_message "#{e.class}: #{e.message}" + mark_message e.class.name mark_failure end @@ -35,18 +35,18 @@ def iiif_test_uri ) end - # Returns a hash with :message and :warning keys + # Returns a hash with :message and :failure keys def validate_iiif_server test_uri = iiif_test_uri - return { message: 'Unable to construct test image URI', warning: true } unless test_uri + return { message: 'Unable to construct test image URI', failure: true } unless test_uri response = iiif_connection.head(test_uri) - return { message: "HEAD #{test_uri} returned status #{response.status}", warning: true } unless response.success? + return { message: "HEAD #{test_uri} returned status #{response.status}", failure: true } unless response.success? acao_header = response.headers['Access-Control-Allow-Origin'] - return { message: "HEAD #{test_uri} missing Access-Control-Allow-Origin header", warning: true } if acao_header.blank? + return { message: "HEAD #{test_uri} missing Access-Control-Allow-Origin header", failure: true } if acao_header.blank? - { message: 'IIIF server reachable', warning: false } + { message: 'IIIF server reachable', failure: false } end end end diff --git a/app/lib/health_checks/lending_root_path.rb b/app/lib/health_checks/lending_root_path.rb index afab140..fc078c4 100644 --- a/app/lib/health_checks/lending_root_path.rb +++ b/app/lib/health_checks/lending_root_path.rb @@ -5,10 +5,10 @@ class LendingRootPath < OkComputer::Check def check result = validate_lending_root mark_message result[:message] - mark_failure if result[:warning] + mark_failure if result[:failure] rescue StandardError => e logger.error(e) - mark_message "#{e.class}: #{e.message}" + mark_message "Error: #{e.class.name}" mark_failure end @@ -18,14 +18,14 @@ def lending_root @lending_root ||= Lending::Config.lending_root_path end - # Returns a hash with :message and :warning keys + # Returns a hash with :message and :failure keys def validate_lending_root - return { message: 'Lending root path not set', warning: true } unless lending_root - return { message: "Not a pathname: #{lending_root.inspect}", warning: true } unless lending_root.is_a?(Pathname) - return { message: "Not a directory: #{lending_root}", warning: true } unless lending_root.directory? - return { message: "Directory not readable: #{lending_root}", warning: true } unless lending_root.readable? + return { message: 'Lending root path not set', failure: true } unless lending_root + return { message: "Not a pathname: #{lending_root.inspect}", failure: true } unless lending_root.is_a?(Pathname) + return { message: "Not a directory: #{lending_root}", failure: true } unless lending_root.directory? + return { message: "Directory not readable: #{lending_root}", failure: true } unless lending_root.readable? - { message: 'Lending root path exists and is readable', warning: false } + { message: 'Lending root path exists and is readable', failure: false } end end end diff --git a/app/lib/health_checks/test_item_exists.rb b/app/lib/health_checks/test_item_exists.rb index 93ed803..84919cc 100644 --- a/app/lib/health_checks/test_item_exists.rb +++ b/app/lib/health_checks/test_item_exists.rb @@ -2,17 +2,16 @@ module HealthChecks class TestItemExists < OkComputer::Check include BerkeleyLibrary::Logging - ERR_NO_COMPLETE_ITEM = 'Unable to locate complete item'.freeze - def check if complete_item mark_message 'Test item lookup succeeded' else - mark_message ERR_NO_COMPLETE_ITEM + mark_message 'Unable to locate complete item' mark_failure end rescue StandardError => e - mark_message "Failed to check item: #{e.message}" + logger.error(e) + mark_message 'Error: failed to check item' mark_failure end diff --git a/spec/lib/health_checks/iiif_server_check_spec.rb b/spec/lib/health_checks/iiif_server_check_spec.rb index 4f61156..c9650e2 100644 --- a/spec/lib/health_checks/iiif_server_check_spec.rb +++ b/spec/lib/health_checks/iiif_server_check_spec.rb @@ -152,7 +152,7 @@ def stub_items(active_first:, inactive_first:) run_check - expect(check.message).to match(/StandardError: boom/) + expect(check.message).to match('StandardError') if check.respond_to?(:failure?) expect(check.failure?).to be(true) @@ -203,15 +203,15 @@ def stub_items(active_first:, inactive_first:) end describe '#validate_iiif_server' do - it 'returns a warning when it cannot construct test uri' do + it 'returns a failure when it cannot construct test uri' do allow(Lending::Config).to receive(:iiif_base_uri).and_return(nil) result = check.send(:validate_iiif_server) - expect(result).to eq(message: 'Unable to construct test image URI', warning: true) + expect(result).to eq(message: 'Unable to construct test image URI', failure: true) end - it 'returns a warning when the HEAD response is not successful' do + it 'returns a failure when the HEAD response is not successful' do base_uri = URI('http://example.test/iiif/') allow(Lending::Config).to receive(:iiif_base_uri).and_return(base_uri) @@ -235,11 +235,11 @@ def stub_items(active_first:, inactive_first:) result = check.send(:validate_iiif_server) - expect(result[:warning]).to be(true) + expect(result[:failure]).to be(true) expect(result[:message]).to match(/returned status 503/) end - it 'returns a warning when Access-Control-Allow-Origin header is missing/blank' do + it 'returns a failure when Access-Control-Allow-Origin header is missing/blank' do base_uri = URI('http://example.test/iiif/') allow(Lending::Config).to receive(:iiif_base_uri).and_return(base_uri) @@ -265,7 +265,7 @@ def stub_items(active_first:, inactive_first:) expect(result).to eq( message: "HEAD #{test_uri} missing Access-Control-Allow-Origin header", - warning: true + failure: true ) end @@ -293,7 +293,7 @@ def stub_items(active_first:, inactive_first:) result = check.send(:validate_iiif_server) - expect(result).to eq(message: 'IIIF server reachable', warning: false) + expect(result).to eq(message: 'IIIF server reachable', failure: false) end end end diff --git a/spec/lib/health_checks/lending_root_path_spec.rb b/spec/lib/health_checks/lending_root_path_spec.rb index 24c8bf7..8cbe1e2 100644 --- a/spec/lib/health_checks/lending_root_path_spec.rb +++ b/spec/lib/health_checks/lending_root_path_spec.rb @@ -78,7 +78,7 @@ def expect_not_failed run_check - expect(check.message).to match(/StandardError: boom/) + expect(check.message).to match('Error: StandardError') expect_failed end end @@ -98,12 +98,12 @@ def expect_not_failed end describe '#validate_lending_root' do - it 'returns a warning when lending root is not a Pathname' do + it 'returns a failure when lending root is not a Pathname' do allow(Lending::Config).to receive(:lending_root_path).and_return('/tmp/not_a_pathname') result = check.send(:validate_lending_root) - expect(result[:warning]).to be(true) + expect(result[:failure]).to be(true) expect(result[:message]).to match(/Not a pathname/) end end diff --git a/spec/lib/health_checks/test_item_exists_spec.rb b/spec/lib/health_checks/test_item_exists_spec.rb index 9a92b88..977d86c 100644 --- a/spec/lib/health_checks/test_item_exists_spec.rb +++ b/spec/lib/health_checks/test_item_exists_spec.rb @@ -45,7 +45,7 @@ def expect_failed run_check - expect(check.message).to eq(described_class::ERR_NO_COMPLETE_ITEM) + expect(check.message).to eq('Unable to locate complete item') expect_failed end @@ -73,7 +73,7 @@ def expect_failed run_check - expect(check.message).to eq('Failed to check item: boom') + expect(check.message).to eq('Error: failed to check item') expect_failed end end diff --git a/spec/lib/lending/config_spec.rb b/spec/lib/lending/config_spec.rb index ad3a048..48b06d8 100644 --- a/spec/lib/lending/config_spec.rb +++ b/spec/lib/lending/config_spec.rb @@ -62,19 +62,19 @@ module Lending Config.instance_variable_set(:@lending_root_path, nil) end - it 'raises ConfigException when ENV IIIF base URL is invalid (covers line 70)' do + it 'raises ConfigException when ENV IIIF base URL is invalid' do ENV[Config::ENV_IIIF_BASE] = 'http://exa mple.org/bad uri' expect { Config.iiif_base_uri }.to raise_error(Lending::ConfigException, /Invalid IIIF base URI:/) end - it 'raises ConfigException when ENV lending root is not a directory (covers lines 76-79 / 77)' do + it 'raises ConfigException when ENV lending root is not a directory' do ENV[Config::ENV_ROOT] = '/definitely/not/a/real/path' expect { Config.lending_root_path }.to raise_error(Lending::ConfigException, /Invalid lending root:/) end - it 'reads IIIF base from Rails config when ENV is unset (covers rails_iiif_base / line 52 and rails_config_value / 85-88)' do + it 'reads IIIF base from Rails config when ENV is unset' do ENV[Config::ENV_IIIF_BASE] = nil Config.instance_variable_set(:@iiif_base_uri, nil) @@ -82,7 +82,7 @@ module Lending expect(Config.iiif_base_uri).to eq(expected) end - it 'reads lending root from Rails config when ENV is unset (covers rails_lending_root / lines 61-62 and rails_config_value / 85-88)' do + it 'reads lending root from Rails config when ENV is unset' do ENV[Config::ENV_ROOT] = nil Config.instance_variable_set(:@lending_root_path, nil) @@ -90,7 +90,7 @@ module Lending expect(Config.lending_root_path).to eq(expected) end - it 'returns nil from rails_config_value when Rails is not defined (covers rails_config / 92-95)' do + it 'returns nil from rails_config_value when Rails is not defined' do ENV[Config::ENV_IIIF_BASE] = nil Config.instance_variable_set(:@iiif_base_uri, nil) @@ -104,7 +104,7 @@ module Lending end end - it 'returns nil from rails_config when Rails.application is nil (covers rails_config / 92-95)' do + it 'returns nil from rails_config when Rails.application is nil' do ENV[Config::ENV_IIIF_BASE] = nil Config.instance_variable_set(:@iiif_base_uri, nil) diff --git a/spec/system/health_system_spec.rb b/spec/system/health_system_spec.rb index e182b99..600a7f3 100644 --- a/spec/system/health_system_spec.rb +++ b/spec/system/health_system_spec.rb @@ -10,29 +10,22 @@ end it 'returns a failure status when a critical service is down' do - # 1. Inject an environment variable into the current process. - # Note: For this to work in Docker, the Puma server must be - # running in the same container or share the ENV. - # A more robust way for Docker is to stub the ActiveRecord call itself: - - allow(ActiveRecord::Base).to receive(:connected?).and_return(false) - # Wait! The above also only works in the test process. - - # INSTEAD: Let's use the Registry to register a temporary - # failing check that doesn't rely on mocks. + failing_check_class = Class.new(OkComputer::Check) do + def check + mark_message 'Intentional failure for test' + mark_failure + end + end begin - # Register a check that always fails - OkComputer::Registry.register 'failing-check', OkComputer::HttpCheck.new('http://localhost:9999/nonexistent') + OkComputer::Registry.register('failing-check', failing_check_class.new) visit '/health' - json = JSON.parse(page.text) - # Verify that at least one check failed - expect(json.values.any? { |v| v['success'] == false }).to be true + + expect(json.dig('failing-check', 'success')).to be false ensure - # Always unregister to avoid bleeding into other tests - OkComputer::Registry.deregister 'failing-check' + OkComputer::Registry.deregister('failing-check') end end end From d37f534f88486b26d2dbc69c286548640d6777f8 Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Wed, 28 Jan 2026 17:49:37 -0800 Subject: [PATCH 6/9] Remove unnecessary system spec and a little cleanup --- CHANGES.md | 4 +++ config/initializers/okcomputer.rb | 1 - .../health_checks/iiif_server_check_spec.rb | 1 - .../health_checks/lending_root_path_spec.rb | 1 - spec/system/health_system_spec.rb | 32 ------------------- 5 files changed, 4 insertions(+), 35 deletions(-) delete mode 100644 spec/system/health_system_spec.rb diff --git a/CHANGES.md b/CHANGES.md index f91f0bb..dac35f8 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,3 +1,7 @@ +# 1.7.6 (xxxx-xx-xx) + +* Implement OKComputer for healthchecks + # 1.7.5 (2025-12-17) * Hotfix for release workflow issue diff --git a/config/initializers/okcomputer.rb b/config/initializers/okcomputer.rb index 366d8ff..21e24ac 100644 --- a/config/initializers/okcomputer.rb +++ b/config/initializers/okcomputer.rb @@ -1,4 +1,3 @@ -# initializers/okcomputer.rb require 'health_checks' OkComputer.logger = Rails.logger diff --git a/spec/lib/health_checks/iiif_server_check_spec.rb b/spec/lib/health_checks/iiif_server_check_spec.rb index c9650e2..6e7e45d 100644 --- a/spec/lib/health_checks/iiif_server_check_spec.rb +++ b/spec/lib/health_checks/iiif_server_check_spec.rb @@ -1,4 +1,3 @@ -# spec/lib/health_checks/iiif_server_check_spec.rb require 'rails_helper' RSpec.describe HealthChecks::IIIFServerCheck do diff --git a/spec/lib/health_checks/lending_root_path_spec.rb b/spec/lib/health_checks/lending_root_path_spec.rb index 8cbe1e2..0ae2e26 100644 --- a/spec/lib/health_checks/lending_root_path_spec.rb +++ b/spec/lib/health_checks/lending_root_path_spec.rb @@ -1,4 +1,3 @@ -# spec/lib/health_checks/lending_root_path_spec.rb require 'rails_helper' RSpec.describe HealthChecks::LendingRootPath do diff --git a/spec/system/health_system_spec.rb b/spec/system/health_system_spec.rb deleted file mode 100644 index 600a7f3..0000000 --- a/spec/system/health_system_spec.rb +++ /dev/null @@ -1,32 +0,0 @@ -require 'rails_helper' - -RSpec.describe 'Health Check', type: :system do - describe 'Checking system health' do - it 'renders the health status as JSON when healthy' do - visit '/health' - json = JSON.parse(page.text) - - expect(json.dig('database', 'success')).to be true - end - - it 'returns a failure status when a critical service is down' do - failing_check_class = Class.new(OkComputer::Check) do - def check - mark_message 'Intentional failure for test' - mark_failure - end - end - - begin - OkComputer::Registry.register('failing-check', failing_check_class.new) - - visit '/health' - json = JSON.parse(page.text) - - expect(json.dig('failing-check', 'success')).to be false - ensure - OkComputer::Registry.deregister('failing-check') - end - end - end -end From 5ae14cca6354f6d190d3707fd4a6090be421f563 Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Thu, 29 Jan 2026 13:17:21 -0800 Subject: [PATCH 7/9] Remove ordered expects from process files --- .yarnrc | 2 +- spec/lib/lending/collector_spec.rb | 32 ++++++++++++++++++++++++++++-- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/.yarnrc b/.yarnrc index cd514f7..5b2cae6 100644 --- a/.yarnrc +++ b/.yarnrc @@ -2,4 +2,4 @@ # yarn lockfile v1 -lastUpdateCheck 1769625849915 +lastUpdateCheck 1769715822620 diff --git a/spec/lib/lending/collector_spec.rb b/spec/lib/lending/collector_spec.rb index b8ab781..75460d4 100644 --- a/spec/lib/lending/collector_spec.rb +++ b/spec/lib/lending/collector_spec.rb @@ -1,5 +1,20 @@ require 'rails_helper' +def capture_logs(logger, levels: %i[info error], pattern: nil) + logs = Hash.new { |h, k| h[k] = [] } + + levels.each do |level| + allow(logger).to receive(level) do |msg, *| + text = msg.to_s + next if pattern && !text.match?(pattern) + + logs[level] << text + end + end + + logs +end + module Lending describe Collector do attr_reader :lending_root @@ -74,14 +89,27 @@ def expect_to_process(item_dirname) end it 'processes files' do - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/starting/).ordered + logs = capture_logs( + BerkeleyLibrary::Logging.logger, + levels: %i[info] + ) + processing_dir, final_dir = expect_to_process('b12345678_c12345678') - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/nothing left to process/).ordered + collector.collect! expect(processing_dir).not_to exist expect(final_dir).to exist expect(collector.stopped?).to eq(false) + + info = logs[:info].join("\n") + + # Removing ordered expects which were very brittle in CI: + expect(info).to match(/starting/) + expect(info).to match(/processing.*b12345678_c12345678/) + expect(info).to match(/moving.*b12345678_c12345678/) + expect(info).to match(/triggering garbage collection/) + expect(info).to match(/nothing left to process/) end # rubocop:disable RSpec/ExampleLength From da619e1183effd23a4a317dc480c2e0788ae2d5e Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Thu, 29 Jan 2026 14:19:19 -0800 Subject: [PATCH 8/9] Remove ALL ordered expects --- spec/lib/lending/collector_spec.rb | 89 +++++++++++++++++------------- 1 file changed, 51 insertions(+), 38 deletions(-) diff --git a/spec/lib/lending/collector_spec.rb b/spec/lib/lending/collector_spec.rb index 75460d4..87ff6a3 100644 --- a/spec/lib/lending/collector_spec.rb +++ b/spec/lib/lending/collector_spec.rb @@ -4,7 +4,7 @@ def capture_logs(logger, levels: %i[info error], pattern: nil) logs = Hash.new { |h, k| h[k] = [] } levels.each do |level| - allow(logger).to receive(level) do |msg, *| + allow(logger).to receive(level) do |msg = nil, *| text = msg.to_s next if pattern && !text.match?(pattern) @@ -55,10 +55,6 @@ def expect_to_process(item_dirname) expect(processing_dir).to exist end - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/processing.*#{item_dirname}/).ordered - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/moving.*#{item_dirname}/).ordered - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/triggering garbage collection/).ordered - [processing_dir, final_dir] end @@ -69,30 +65,33 @@ def expect_to_process(item_dirname) end it 'processes nothing if stopped' do - collector.stop! + logs = capture_logs(BerkeleyLibrary::Logging.logger, levels: %i[info]) - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/starting/).ordered - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/stopped/).ordered + collector.stop! expect(Processor).not_to receive(:new) + collector.collect! + + info = logs[:info].join("\n") + expect(info).to match(/starting/) + expect(info).to match(/stopped/) end it 'stops if a stop file is present' do - stop_file_path = collector.stop_file_path - FileUtils.touch(stop_file_path.to_s) + logs = capture_logs(BerkeleyLibrary::Logging.logger, levels: %i[info]) - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/starting/).ordered - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/stop file .* found/).ordered + FileUtils.touch(collector.stop_file_path.to_s) expect(Processor).not_to receive(:new) collector.collect! + + info = logs[:info].join("\n") + expect(info).to match(/starting/) + expect(info).to match(/stop file .* found/) end it 'processes files' do - logs = capture_logs( - BerkeleyLibrary::Logging.logger, - levels: %i[info] - ) + logs = capture_logs(BerkeleyLibrary::Logging.logger, levels: %i[info]) processing_dir, final_dir = expect_to_process('b12345678_c12345678') @@ -117,12 +116,11 @@ def expect_to_process(item_dirname) processing_dirs = [] final_dirs = [] - logger = BerkeleyLibrary::Logging.logger - logs = [] - - allow(logger).to receive(:info) do |msg| - logs << msg if msg.match?(/starting|nothing left to process/) - end + logs = capture_logs( + BerkeleyLibrary::Logging.logger, + levels: %i[info], + pattern: /starting|nothing left to process/ + ) %w[b12345678_c12345678 b86753090_c86753090].each do |item_dir| pdir, fdir = expect_to_process(item_dir) @@ -132,21 +130,27 @@ def expect_to_process(item_dirname) collector.collect! - start_index = logs.index { |l| l =~ /starting/ } - end_index = logs.index { |l| l =~ /nothing left to process/ } + info_lines = logs[:info] + expect(info_lines.grep(/starting/)).not_to be_empty + expect(info_lines.grep(/nothing left to process/)).not_to be_empty + + start_index = info_lines.index { |l| l =~ /starting/ } + end_index = info_lines.index { |l| l =~ /nothing left to process/ } expect(start_index).not_to be_nil expect(end_index).not_to be_nil expect(start_index).to be < end_index processing_dirs.each { |pdir| expect(pdir).not_to exist } - final_dirs.each { |fdir| expect(fdir).to exist } + final_dirs.each { |fdir| expect(fdir).to exist } expect(collector.stopped?).to eq(false) end # rubocop:enable RSpec/ExampleLength # rubocop:disable RSpec/ExampleLength it 'skips single-item processing failures' do + logs = capture_logs(BerkeleyLibrary::Logging.logger, levels: %i[info error]) + bad_item_dir = 'b12345678_c12345678' bad_ready_dir = lending_root.join('ready').join(bad_item_dir) @@ -163,21 +167,24 @@ def expect_to_process(item_dirname) error_message = 'Oops' expect(bad_processor).to(receive(:process!)).and_raise(error_message) - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/starting/).ordered - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/processing/).ordered - expect(BerkeleyLibrary::Logging.logger).to receive(:error).with(/Processing.*failed/, an_object_satisfying do |obj| - obj.is_a?(Lending::ProcessingFailed) - obj.message.include?(error_message) - end).ordered - # GC.start should be called even if processing fails - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/triggering garbage collection/).ordered - good_item_dir = 'b86753090_c86753090' good_processing_dir, good_final_dir = expect_to_process(good_item_dir) + collector.collect! - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/nothing left to process/).ordered + info = logs[:info].join("\n") - collector.collect! + expect(info).to match(/starting/) + expect(info).to match(/processing.*#{bad_item_dir}/) + + error_text = logs[:error].join("\n") + + expect(error_text).to match(/Processing.*failed/) + expect(error_text).to include(error_message) + + # GC.start should be called even if processing fails + expect(info).to match(/triggering garbage collection/) + + expect(info).to match(/nothing left to process/) expect(bad_processing_dir).to exist expect(bad_final_dir).not_to exist @@ -190,11 +197,17 @@ def expect_to_process(item_dirname) # rubocop:enable RSpec/ExampleLength it 'exits cleanly in the event of some random error' do + logs = capture_logs(BerkeleyLibrary::Logging.logger, levels: %i[info error]) + FileUtils.remove_dir(lending_root.to_s) - expect(BerkeleyLibrary::Logging.logger).to receive(:info).with(/starting/).ordered - expect(BerkeleyLibrary::Logging.logger).to receive(:error).with(/exiting due to error/, a_kind_of(StandardError)) collector.collect! + + info = logs[:info].join("\n") + error = logs[:error].join("\n") + + expect(info).to match(/starting/) + expect(error).to match(/exiting due to error/) end end From 81eef2d0cbb531068a8a8be9dc73989d4a82fbde Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Tue, 3 Feb 2026 12:32:07 -0800 Subject: [PATCH 9/9] Update change log --- .yarnrc | 2 +- CHANGES.md | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.yarnrc b/.yarnrc index 5b2cae6..f5b1384 100644 --- a/.yarnrc +++ b/.yarnrc @@ -2,4 +2,4 @@ # yarn lockfile v1 -lastUpdateCheck 1769715822620 +lastUpdateCheck 1770150293563 diff --git a/CHANGES.md b/CHANGES.md index dac35f8..7b48cce 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,6 +1,7 @@ -# 1.7.6 (xxxx-xx-xx) +# 1.7.8 (xxxx-xx-xx) * Implement OKComputer for healthchecks +* Refactor collector_spec to not include brittle ".ordered" expectations # 1.7.5 (2025-12-17)