Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .rubocop.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# This file is based on https://github.com/rails/rails/blob/master/.rubocop.yml (MIT license)
# Automatically generated by OpenAPI Generator (https://openapi-generator.tech)
AllCops:
TargetRubyVersion: 2.4
TargetRubyVersion: 3.3
# RuboCop has a bunch of cops enabled by default. This setting tells RuboCop
# to ignore them, so only the ones explicitly set in this file are enabled.
DisabledByDefault: true
Expand Down
4 changes: 2 additions & 2 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@ gemspec
group :development, :test do
gem 'rake', '~> 13.2.1'
gem 'pry-byebug'
gem 'rubocop', '~> 1.82.0'
gem 'simplecov', '~> 0.21.2'
gem 'rubocop', '~> 1.86.2'
gem 'simplecov', '~> 0.22.0'
end
319 changes: 319 additions & 0 deletions UPDATING_MODEL_TESTS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,319 @@
# Updating Model Unit Tests

## Context

Model unit test files in `spec/unit/models/` are auto-generated by the `openapi-generator-cli` (`rake generate` or equivalent). The generator output is a *starting point* — every spec must be updated to match the template described here.

This doc is the source of truth for that update step. Follow it exactly when bringing a fresh-generated spec into compliance.

## Workflow

1. Auto-generate the SDK (writes specs to `spec/unit/models/`).
2. For each spec under `spec/unit/models/`, pick the matching template:
- [Standard Model Template](#standard-model-template) — regular models (the model file defines a `class`)
- [Enum Model Template](#enum-model-template) — pure enum models (the model file defines a class with `all_vars` and frozen string constants only)
- [oneOf Model Template](#oneof-model-template) — discriminator-based oneOf models (the model file defines a `module`, not a `class`)
3. Run `rake unit` and confirm `0 failures`.
4. Check coverage didn't regress meaningfully (see [Coverage notes](#coverage-notes)).

## Standard Model Template

Apply to any spec representing a regular model (object with attributes), e.g. `message_spec.rb`, `call_state_spec.rb`, `verify_code_request_spec.rb`.

### Required structure

```ruby
describe Bandwidth::ModelName do
let(:model_name_default) { Bandwidth::ModelName.new }
let(:model_name_values) { Bandwidth::ModelName.new({ <all attrs populated> }) }

describe '#initialize' do
it 'causes an ArgumentError by passing an Array to the initialize method'
it 'causes an ArgumentError by passing an invalid attribute to the initialize method'
end

describe '#acceptable_attributes' do
it 'expects acceptable JSON attributes to be those in the attribute map'
end

describe '#openapi_nullable' do
# If model has nullable attrs, assert the exact Set. Otherwise assert empty Set.
it 'expects nullable attributes to be ...'
end

describe '#build_from_hash' do
# camelCase keys in, snake_case attr reads out, instance check.
it 'validates instance of ModelName created by the build_from_hash method'
end

describe '#to_s' do
# Use the *populated* values instance, not the empty default.
it 'returns a string representation of the object'
end

describe '#eq? #==' do
# Assert both equality AND inequality.
it 'returns true/false when comparing objects'
end

describe '#to_body #to_hash' do
it 'returns a hash representation of the object'
end

# Only when the model has validated attrs (nil/pattern/min/max).
describe 'custom attribute writers' do
it '#attr=' do ... end
end
end
```

### What to drop from the auto-generated spec

- **`#hash` block.** Asserting `obj.hash.is_a?(Integer)` is testing Ruby itself. No signal.
- **Empty `enum validation` block** (e.g. `describe 'enum validation' do; it 'works' do; end; end`). Pure noise — passes while testing nothing.
- **`EnumAttributeValidator` block.** It tests generator scaffolding with fake allow-lists (`['valid']`, `[1]`), not the model's actual enums. The customer-facing behavior — "does an invalid enum value raise when set on a model?" — belongs in custom attribute writer tests, where it actually exercises the model.

### What to add when missing

- **`_default` let.** Pair `_values` with a default instance so `#eq?` can assert both equal-to-equal and equal-to-different. For models with required (non-nil-validated) attrs, build `_default` with only the required attrs populated.
- **`#openapi_nullable` block.** Always include, even if the nullable set is empty. Read the model's `self.openapi_nullable` to get the exact set.
- **Custom attribute writer tests.** One `it` per validated attribute. Cover every validation the model performs:
- `nil` check → `'<attr> cannot be nil'`
- pattern → `'invalid value for "<attr>", must conform to the pattern ...'`
- min/max length → `'invalid value for "<attr>", the character length must be ...'`
- min/max value → `'invalid value for "<attr>", must be smaller/greater than or equal to ...'`
- enum allow-list → `'invalid value for "<attr>", must be one of ...'`

### What to fix when incorrect

- **`#to_s` using the empty `_default` instance.** Switch to `_values` so the assertion is meaningful (asserts `'{}'` proves nothing).
- **`#eq?` only checking equality.** Update to assert both `_default.eql?(equal_instance)` is true AND `_default.eql?(_values)` is false.

## Enum Model Template

Apply to pure enum models, e.g. `call_state_enum_spec.rb`.

```ruby
describe Bandwidth::SomeEnum do
describe 'constants' do
it 'defines VALUE_NAME' do
expect(Bandwidth::SomeEnum::VALUE_NAME).to eq('value_string')
end
# ...one per enum constant
end

describe '.all_vars' do
it 'returns every valid enum value' do
expect(Bandwidth::SomeEnum.all_vars).to eq([<every value in order>])
end
end

describe '.build_from_hash' do
it 'returns the value when it matches a valid enum value' do
expect(Bandwidth::SomeEnum.build_from_hash('valid_value')).to eq('valid_value')
# ...one per valid value
end

it 'raises an error for an invalid enum value' do
expect { Bandwidth::SomeEnum.build_from_hash('invalid') }.to raise_error(RuntimeError)
end
end
end
```

### What to drop

- Instance-construction tests (`Bandwidth::SomeEnum.new` then `be_instance_of`). The enum module isn't meant to be instantiated for use — only `build_from_hash` matters.

## oneOf Model Template

Apply to discriminator-based oneOf models, e.g. `callback_spec.rb`. These models are defined as Ruby `module`s (not classes) and act as routers — given a payload with a discriminator field, they dispatch to one of several target classes via `.build`.

You can identify a oneOf model by inspecting `lib/bandwidth-sdk/models/<name>.rb`: it will be a `module` exposing `openapi_one_of`, `openapi_discriminator_name`, `openapi_discriminator_mapping`, and `build` as class methods.

```ruby
describe Bandwidth::SomeOneOf do
describe '.openapi_one_of' do
it 'lists the classes defined in oneOf' do
expect(Bandwidth::SomeOneOf.openapi_one_of).to eq([
:'ClassA',
:'ClassB'
])
end
end

describe '.openapi_discriminator_name' do
it 'returns the discriminator property name' do
expect(Bandwidth::SomeOneOf.openapi_discriminator_name).to eq(:'type')
end
end

describe '.openapi_discriminator_mapping' do
it 'maps every discriminator value to a oneOf class' do
expect(Bandwidth::SomeOneOf.openapi_discriminator_mapping).to eq({
:'discriminator-value-1' => :'ClassA',
:'discriminator-value-2' => :'ClassB'
})
end

it 'maps only to classes listed in openapi_one_of' do
mapping_targets = Bandwidth::SomeOneOf.openapi_discriminator_mapping.values.uniq.sort
expect(mapping_targets).to eq(Bandwidth::SomeOneOf.openapi_one_of.sort)
end
end

describe '.build' do
# Stub build_from_hash on each target class so we test the routing without
# depending on the target's construction logic (which is covered by its own spec).
it 'routes <ClassA> discriminator values to ClassA.build_from_hash' do
Bandwidth::SomeOneOf.openapi_discriminator_mapping.each do |discriminator, klass|
next unless klass == :'ClassA'
data = { type: discriminator.to_s }
expect(Bandwidth::ClassA).to receive(:build_from_hash).with(data).and_return(:class_a_result)
expect(Bandwidth::SomeOneOf.build(data)).to eq(:class_a_result)
end
end

# ...one block per oneOf target class

it 'returns nil when the discriminator value is missing' do
expect(Bandwidth::SomeOneOf.build({})).to be_nil
end

it 'returns nil when the discriminator value does not match any mapping' do
expect(Bandwidth::SomeOneOf.build({ type: 'unknown' })).to be_nil
end
end
end
```

### Required structure

- `.openapi_one_of` — assert the exact list of target class symbols
- `.openapi_discriminator_name` — assert the exact discriminator property symbol
- `.openapi_discriminator_mapping` — assert the exact mapping AND assert it only maps to classes in `openapi_one_of`
- `.build` — one block per target class verifying routing (via stubbed `build_from_hash`), plus the two nil cases (missing discriminator, unknown discriminator value)

### What to drop from the auto-generated spec

- **Weak `not_to be_empty` assertions.** Replace with exact equality checks against the model's actual values.
- **`mapping.values.sort == one_of.sort` assertion.** This is buggy in the generator output — `mapping.values` has duplicates when multiple discriminator values route to the same class. Use `.uniq.sort` instead.
- **Empty `.build` describe block.** Replace with the stub-based routing tests above.

### Why stub `build_from_hash`?

oneOf targets often have many required attributes (and nested required models), making it tedious to construct valid data for every discriminator value. Stubbing isolates the test to what `.build` is actually doing — routing based on a discriminator. The target class's `build_from_hash` is covered by that class's own spec.

### What to skip

- `#initialize`, `#openapi_nullable`, `#to_s`, `#eq?`, `#to_body`, custom attribute writers — these don't apply. The oneOf model is a `module`, not an instantiable class.

### Variant: discriminator-based `openapi_any_of`

Some modules expose `openapi_any_of` instead of `openapi_one_of` but otherwise have the identical shape (same `openapi_discriminator_name`, `openapi_discriminator_mapping`, and `build` implementation). Treat them exactly like the oneOf template above — just substitute `openapi_any_of` for `openapi_one_of` everywhere (describe block name, assertion target, the `uniq.sort` cross-check).

Example: `multi_channel_action_spec.rb` (the model is `lib/bandwidth-sdk/models/multi_channel_action.rb`).

### Variant: no-discriminator `openapi_one_of`

Some `oneOf` modules expose `openapi_one_of` but **no** discriminator (no `openapi_discriminator_name`/`openapi_discriminator_mapping`). Instead, `build(data)` iterates `openapi_one_of` and uses a private `find_and_cast_into_type` helper to attempt deserialization into each target class in order, returning the first match (or `nil` if none match).

You **cannot use the stubbed-routing pattern** for these — `find_and_cast_into_type` does a pre-flight check against the target's `acceptable_attributes` before calling `build_from_hash`, so a naked stub of `build_from_hash` is bypassed. Use real payloads instead, sized to match one target's attribute shape.

```ruby
describe Bandwidth::SomeNoDiscriminatorOneOf do
describe '.openapi_one_of' do
it 'lists the classes defined in oneOf' do
expect(Bandwidth::SomeNoDiscriminatorOneOf.openapi_one_of).to eq([
:'ClassA',
:'ClassB'
])
end
end

describe '.build' do
it 'routes payloads matching ClassA attributes to ClassA' do
data = { someClassAAttr: 'value' }
expect(Bandwidth::SomeNoDiscriminatorOneOf.build(data)).to be_instance_of(Bandwidth::ClassA)
end

it 'routes payloads matching ClassB attributes to ClassB' do
data = { someClassBAttr: 'value' }
expect(Bandwidth::SomeNoDiscriminatorOneOf.build(data)).to be_instance_of(Bandwidth::ClassB)
end

it 'returns nil when the payload does not match any oneOf schema' do
expect(Bandwidth::SomeNoDiscriminatorOneOf.build({ unknown: 'value' })).to be_nil
end
end
end
```

Pick payload attributes that exist on only one target so `find_and_cast_into_type`'s `acceptable_attributes` check disambiguates correctly. Skip `.openapi_discriminator_name` and `.openapi_discriminator_mapping` blocks — those methods don't exist on these modules.

Example: `rbm_message_content_rich_card_spec.rb`.

## Gotchas

### Writers unreachable through `Model.new`

Some validated setters (notably `to=` and `media=` on `Bandwidth::Message`) only get invoked from `initialize` when the input is an Array. Passing `nil` to `Model.new` won't trigger the writer's nil check.

**Fix:** call the writer directly on an instance.

```ruby
# Wrong — won't raise:
expect { Bandwidth::Message.new({ to: nil }) }.to raise_error(ArgumentError, 'to cannot be nil')

# Right — exercises the writer:
expect { message_values.to = nil }.to raise_error(ArgumentError, 'to cannot be nil')
```

When in doubt, read the model's `initialize` and check whether `self.<attr> =` is gated behind a type check.

### `#build_from_hash` needs full required-attr data for nested models

When a model has an attribute typed as another model (e.g., `:'business_address' => :'Address'`), `build_from_hash` recursively deserializes that nested hash into a real `Address` instance. That instance's `initialize` runs all required-attr setters, so any missing required attr raises `ArgumentError`.

**This bites only in the `#build_from_hash` test, not `#to_s` / `#to_body` / `_values`** — when you pass a nested hash to `Model.new` directly (via `_values`), the parent's `attr_accessor` just stores the hash verbatim; no recursive deserialization happens.

```ruby
# Wrong — raises ArgumentError: 'addr1 cannot be nil' inside Address.new:
Bandwidth::VerificationRequest.build_from_hash({
businessAddress: { name: 'Bandwidth' }, # Address requires addr1, city, state, zip, url
# ...
})

# Right — full required-attr data for the nested Address:
Bandwidth::VerificationRequest.build_from_hash({
businessAddress: { name: 'Bandwidth', addr1: '900 Main Campus Dr', city: 'Raleigh', state: 'NC', zip: '27606', url: 'https://www.bandwidth.com' },
# ...
})
```

Apply this recursively — if the nested model itself has nested models with required attrs, fill those too. Check the nested model's setters (`def <attr>=` blocks raising `'<attr> cannot be nil'`) to know which attrs are required.

### `#to_s` is sensitive to attribute order

The expected string must match the exact order of attributes as serialized by `to_hash`. Generate the expected string by running the populated instance through `to_s` in a console and pasting the result. Don't hand-craft it.

### Nullable attribute symbols

`openapi_nullable` returns a `Set` of `Symbol`s using snake_case attribute names with a leading colon-quote (e.g. `:'parent_call_id'`). Mirror the model's `openapi_nullable` definition exactly.

## Coverage notes

Dropping `EnumAttributeValidator` blocks reduces line coverage slightly, because the nested `EnumAttributeValidator` class isn't exercised elsewhere. Two options:

1. **Accept the dip.** That code is generator scaffolding; coverage of it is low-value.
2. **Add an enum-validated writer test** for at least one attribute per model with enums (e.g. `Bandwidth::Message.new(direction: 'invalid')` should raise). This exercises `EnumAttributeValidator` through a real path and recovers the coverage.

The coverage logic itself is being updated as part of expanding to all models — don't fight stale thresholds.

## Verification

```sh
rake unit
```

Confirm `0 failures` before considering a spec done.
4 changes: 2 additions & 2 deletions custom_templates/Gemfile.mustache
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@ gemspec
group :development, :test do
gem 'rake', '~> 13.2.1'
gem 'pry-byebug'
gem 'rubocop', '~> 1.82.0'
gem 'simplecov', '~> 0.21.2'
gem 'rubocop', '~> 1.86.2'
gem 'simplecov', '~> 0.22.0'
end
16 changes: 16 additions & 0 deletions generate-model-tests.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#!/bin/bash

# Generates new test files for models. Run from the root.

# allow generator to write test files
sed -i.bak 's|^/spec/\*\*|# /spec/**|' .openapi-generator-ignore && rm .openapi-generator-ignore.bak
# remove current test files for models
rm -f ./spec/unit/models/*
# generate new test files for models
openapi-generator-cli generate -i bandwidth.yml -o ./ -c openapi-config.yml -g ruby > /dev/null
# move generated model test files to the correct location (exclude api tests)
mv ./spec/models/* ./spec/unit/models/
# remove remaining generated test files (api tests, etc.)
rm -rf ./spec/api ./spec/models
# discard changes to modified files only (leaves deletions and new test files intact)
modified=$(git diff --name-only --diff-filter=M) && [ -n "$modified" ] && echo "$modified" | xargs git checkout --
1 change: 1 addition & 0 deletions lib/bandwidth-sdk/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ def initialize
body = JSON.parse(response.body)
@access_token = body['access_token']
@access_token_expiration = Time.now + body['expires_in']
@access_token
}

yield(self) if block_given?
Expand Down
Loading
Loading