Forbid bare HTTP status checks in tests (hides debugging

Home Epics Forbid bare HTTP status checks in tests (hides debugging info)Edit

Forbid bare HTTP status checks in tests (hides debugging info)

Cancel

Title *

Project *

Description

Add runtime enforcement to prevent `expect(response).to have_http_status(:ok)` in tests. When agents only check HTTP status, they miss the response body which often contains critical debugging clues.

## Problem

**Bad pattern:**
```ruby
expect(response).to have_http_status(:ok)
# ❌ If this fails, agent sees NO context about why
```

**Better pattern:**
```ruby
expect(response.body).to include("success")
# ✅ If this fails, agent sees the actual response body
```

When a status check fails, the agent only sees:
```
Failure/Error: expect(response).to have_http_status(:ok)
       expected: 200
       got: 500
```

But they DON'T see the response body which might say:
```json
{"error": "Undefined method 'foo' on nil:NilClass", "backtrace": "..."}
```

## Solution

### 1. Monkeypatch RSpec to Forbid Status-Only Checks

**File:** `spec/spec_helper.rb` or `spec/support/response_matchers.rb`

```ruby
# Forbid bare have_http_status checks
RSpec::Matchers.configure do |config|
  config.alias_matcher :have_http_status_banned, :have_http_status
end

module RSpec
  module Matchers
    alias_method :have_http_status_old, :have_http_status
    
    def have_http_status(*args)
      # Detect if called without body check context
      caller_trace = caller.join("\n")
      
      # If this looks like a bare status check without body assertion
      if bare_status_check?(caller_trace)
        warn "\n" + "="*80
        warn "⚠️  BARE HTTP STATUS CHECK DETECTED"
        warn "="*80
        warn "You're checking HTTP status without also checking response body."
        warn "This hides debugging information!"
        warn ""
        warn "Instead of:"
        warn "  expect(response).to have_http_status(:ok)"
        warn ""
        warn "Use:"
        warn "  expect(response.body).to include('expected_content')"
        warn "  # Or for status + body:"
        warn "  expect(response).to have_http_status(:ok)"
        warn "  expect(response.body).to include('success')"
        warn "="*80 + "\n\n"
      end
      
      have_http_status_old(*args)
    end
    
  private
    
    def bare_status_check?(trace)
      # Heuristic: if next line after expect() isn't checking body
      # This is a simple check - can be improved
      trace.any? { |line| line.include?('have_http_status') } &&
        !trace.any? { |line| line.include?('response.body') }
    end
  end
end
```

### 2. Add Custom Matcher That Includes Body

```ruby
# spec/support/response_matchers.rb

RSpec::Matchers.define :have_successful_response do |expected_body_part|
  match do |actual|
    # Check status AND body
    actual.status == 200 && actual.body.include?(expected_body_part)
  end
  
  failure_message do |actual|
    "Expected successful response containing '#{expected_body_part}'\n" \
    "Got status: #{actual.status}\n" \
    "Body: #{actual.body}"
  end
end

# Usage:
expect(response).to have_successful_response("user created")
```

### 3. Add RShared Context for API Tests

```ruby
# spec/support/api_context.rb

RSpec.shared_context "api_response" do
  def expect_api_success(response, expected_content = nil)
    expect(response.status).to eq(200),
      "Expected 200, got #{response.status}. Body: #{response.body}"
    
    if expected_content
      expect(response.body).to include(expected_content),
        "Expected body to include '#{expected_content}'. Body: #{response.body}"
    end
  end
  
  def expect_api_error(response, expected_status, expected_error)
    expect(response.status).to eq(expected_status),
      "Expected #{expected_status}, got #{response.status}. Body: #{response.body}"
    
    expect(response.body).to include(expected_error),
      "Expected error '#{expected_error}'. Body: #{response.body}"
  end
end

# Usage in tests:
expect_api_success(response, "user created")
expect_api_error(response, 422, "Email is required")
```

### 4. Lint Rule (Optional but Better)

**Add RuboCop custom cop:** `spec/rubocop/rspec/status_only_check.rb`

```ruby
module RuboCop
  module Cop
    module RSpec
      class StatusOnlyCheck < Base
        MSG = 'Use expect_api_success or check response.body instead of bare have_http_status'
        
        def_node_matcher :bare_status_check, <<~PATTERN
          (send (send nil? :expect) :to (send nil? :have_http_status ...))
        PATTERN
        
        def on_send(node)
          return unless bare_status_check?(node)
          
          # Check if next expectation isn't checking body
          unless sibling_checks_body?(node)
            add_offense(node, message: MSG)
          end
        end
        
        def sibling_checks_body?(node)
          # Look for sibling expect() that checks response.body
          parent = node.parent
          return false unless parent
          
          parent.children.any? do |sibling|
            next if sibling == node
            sibling.source.include?('response.body')
          end
        end
      end
    end
  end
end
```

## Training/Memory Update

Store memory about this pattern:
```ruby
# Title: "Test anti-pattern: bare HTTP status checks"
# Type: instruction
# Content: "Never use expect(response).to have_http_status(:ok) alone.
#          Always check response.body for better debugging.
#          Use expect_api_success(response, 'expected content') helper instead."
```

## Acceptance Criteria
- [ ] Monkeypatch in spec_helper warns on bare `have_http_status` checks
- [ ] Warning message shows example of better pattern
- [ ] Custom matcher `have_successful_response` added
- [ ] Shared context `expect_api_success` helper available
- [ ] Memory stored instructing agents to avoid this pattern
- [ ] Optional: RuboCop cop enforces this in CI