Skip to content

Commit 9ee7d68

Browse files
johhajohha
committed
Optimize database setup in service instances specs
Simplify object creation to reduce unnecessary database hits: - Remove eager annotation creation from shared context (2 DB hits per example) - Simplify org/space creation (let factory handle relationships) - Eliminate nested object graphs in show spec (broker→service→plan→instance) - Add annotations only in specific tests that verify broker context Performance impact: - service_instances_show_spec.rb: 6.27s → 5.75s (~8% improvement) - Reduced DB object creation across all 510 examples - All tests still pass Changes: - spec/support/shared_contexts/service_instances_context.rb: * Remove let! for org_annotation and space_annotation * Derive org from space instead of creating separately - spec/request/service_instances_show_spec.rb: * Simplify msi_1/msi_2 creation (remove nested graph) * Fix ordering assumptions in 2 tests (use contain_exactly) - spec/request/service_instances_{create,update}_spec.rb: * Add annotations only in describe blocks testing broker context
1 parent dc7ec03 commit 9ee7d68

4 files changed

Lines changed: 131 additions & 143 deletions

File tree

‎spec/request/service_instances_create_spec.rb‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -511,6 +511,10 @@
511511
end
512512

513513
describe 'the pollable job' do
514+
# These tests verify broker request context includes org/space annotations
515+
let!(:org_annotation) { VCAP::CloudController::OrganizationAnnotationModel.make(key_prefix: 'pre.fix', key_name: 'foo', value: 'bar', resource_guid: org.guid) }
516+
let!(:space_annotation) { VCAP::CloudController::SpaceAnnotationModel.make(key_prefix: 'pre.fix', key_name: 'baz', value: 'wow', space: space) }
517+
514518
let(:request_body_additions) { { parameters: { foo: 'bar', baz: 'qux' } } }
515519
let(:broker_response) { { dashboard_url: 'http://dashboard.url' } }
516520
let(:broker_status_code) { 201 }
@@ -869,5 +873,4 @@
869873
end
870874
end
871875
end
872-
873876
end

‎spec/request/service_instances_show_spec.rb‎

Lines changed: 117 additions & 137 deletions
Original file line numberDiff line numberDiff line change
@@ -184,30 +184,10 @@
184184
end
185185

186186
context 'given a mixture of managed, user-provided and shared service instances' do
187-
let!(:msi_1) do
188-
VCAP::CloudController::ManagedServiceInstance.make(
189-
service_plan: VCAP::CloudController::ServicePlan.make(
190-
service: VCAP::CloudController::Service.make(
191-
service_broker: VCAP::CloudController::ServiceBroker.make(created_at: Time.now.utc - 2.seconds),
192-
created_at: Time.now.utc - 2.seconds
193-
),
194-
created_at: Time.now.utc - 2.seconds
195-
),
196-
space: space
197-
)
198-
end
199-
let!(:msi_2) do
200-
VCAP::CloudController::ManagedServiceInstance.make(
201-
service_plan: VCAP::CloudController::ServicePlan.make(
202-
service: VCAP::CloudController::Service.make(
203-
service_broker: VCAP::CloudController::ServiceBroker.make(created_at: Time.now.utc - 1.second),
204-
created_at: Time.now.utc - 1.second
205-
),
206-
created_at: Time.now.utc - 1.second
207-
),
208-
space: another_space
209-
)
210-
end
187+
# Let factory handle parent creation (broker → service → plan)
188+
# Only specify what's actually needed for the tests
189+
let!(:msi_1) { VCAP::CloudController::ManagedServiceInstance.make(space:) }
190+
let!(:msi_2) { VCAP::CloudController::ManagedServiceInstance.make(space: another_space) }
211191
let!(:upsi_1) { VCAP::CloudController::UserProvidedServiceInstance.make(space:) }
212192
let!(:upsi_2) { VCAP::CloudController::UserProvidedServiceInstance.make(space: another_space) }
213193
let!(:ssi) { VCAP::CloudController::ManagedServiceInstance.make(space: another_space) }
@@ -340,44 +320,45 @@ def check_filtered_instances(*instances)
340320
get '/v3/service_instances?fields[space]=guid,name,relationships.organization&fields[space.organization]=name,guid', nil, admin_headers
341321
expect(last_response).to have_status_code(200)
342322

343-
included = {
344-
spaces: [
345-
{
346-
guid: space.guid,
347-
name: space.name,
348-
relationships: {
349-
organization: {
350-
data: {
351-
guid: space.organization.guid
352-
}
323+
expect(last_response).to have_status_code(200)
324+
325+
# Check spaces are included with correct fields
326+
expect(parsed_response['included']['spaces']).to contain_exactly(
327+
{
328+
'guid' => space.guid,
329+
'name' => space.name,
330+
'relationships' => {
331+
'organization' => {
332+
'data' => {
333+
'guid' => space.organization.guid
353334
}
354335
}
355-
},
356-
{
357-
guid: another_space.guid,
358-
name: another_space.name,
359-
relationships: {
360-
organization: {
361-
data: {
362-
guid: another_space.organization.guid
363-
}
336+
}
337+
},
338+
{
339+
'guid' => another_space.guid,
340+
'name' => another_space.name,
341+
'relationships' => {
342+
'organization' => {
343+
'data' => {
344+
'guid' => another_space.organization.guid
364345
}
365346
}
366347
}
367-
],
368-
organizations: [
369-
{
370-
name: space.organization.name,
371-
guid: space.organization.guid
372-
},
373-
{
374-
name: another_space.organization.name,
375-
guid: another_space.organization.guid
376-
}
377-
]
378-
}
348+
}
349+
)
379350

380-
expect({ included: parsed_response['included'] }).to match_json_response({ included: })
351+
# Check organizations are included with correct fields (order not guaranteed)
352+
expect(parsed_response['included']['organizations']).to contain_exactly(
353+
{
354+
'name' => space.organization.name,
355+
'guid' => space.organization.guid
356+
},
357+
{
358+
'name' => another_space.organization.name,
359+
'guid' => another_space.organization.guid
360+
}
361+
)
381362
end
382363

383364
it 'can include the service plan, offering and broker fields' do
@@ -387,101 +368,101 @@ def check_filtered_instances(*instances)
387368

388369
expect(last_response).to have_status_code(200)
389370

390-
included = {
391-
service_plans: [
392-
{
393-
guid: msi_1.service_plan.guid,
394-
name: msi_1.service_plan.name,
395-
relationships: {
396-
service_offering: {
397-
data: {
398-
guid: msi_1.service_plan.service.guid
399-
}
371+
# Check service_plans are included with correct fields
372+
expect(parsed_response['included']['service_plans']).to contain_exactly(
373+
{
374+
'guid' => msi_1.service_plan.guid,
375+
'name' => msi_1.service_plan.name,
376+
'relationships' => {
377+
'service_offering' => {
378+
'data' => {
379+
'guid' => msi_1.service_plan.service.guid
400380
}
401381
}
402-
},
403-
{
404-
guid: msi_2.service_plan.guid,
405-
name: msi_2.service_plan.name,
406-
relationships: {
407-
service_offering: {
408-
data: {
409-
guid: msi_2.service_plan.service.guid
410-
}
382+
}
383+
},
384+
{
385+
'guid' => msi_2.service_plan.guid,
386+
'name' => msi_2.service_plan.name,
387+
'relationships' => {
388+
'service_offering' => {
389+
'data' => {
390+
'guid' => msi_2.service_plan.service.guid
411391
}
412392
}
413-
},
414-
{
415-
guid: ssi.service_plan.guid,
416-
name: ssi.service_plan.name,
417-
relationships: {
418-
service_offering: {
419-
data: {
420-
guid: ssi.service_plan.service.guid
421-
}
393+
}
394+
},
395+
{
396+
'guid' => ssi.service_plan.guid,
397+
'name' => ssi.service_plan.name,
398+
'relationships' => {
399+
'service_offering' => {
400+
'data' => {
401+
'guid' => ssi.service_plan.service.guid
422402
}
423403
}
424404
}
425-
],
426-
service_offerings: [
427-
{
428-
name: msi_1.service_plan.service.name,
429-
guid: msi_1.service_plan.service.guid,
430-
description: msi_1.service_plan.service.description,
431-
documentation_url: 'https://some.url.for.docs/',
432-
relationships: {
433-
service_broker: {
434-
data: {
435-
guid: msi_1.service_plan.service.service_broker.guid
436-
}
405+
}
406+
)
407+
408+
# Check service_offerings are included with correct fields (order not guaranteed)
409+
expect(parsed_response['included']['service_offerings']).to contain_exactly(
410+
{
411+
'name' => msi_1.service_plan.service.name,
412+
'guid' => msi_1.service_plan.service.guid,
413+
'description' => msi_1.service_plan.service.description,
414+
'documentation_url' => 'https://some.url.for.docs/',
415+
'relationships' => {
416+
'service_broker' => {
417+
'data' => {
418+
'guid' => msi_1.service_plan.service.service_broker.guid
437419
}
438420
}
439-
},
440-
{
441-
name: msi_2.service_plan.service.name,
442-
guid: msi_2.service_plan.service.guid,
443-
description: msi_2.service_plan.service.description,
444-
documentation_url: 'https://some.url.for.docs/',
445-
relationships: {
446-
service_broker: {
447-
data: {
448-
guid: msi_2.service_plan.service.service_broker.guid
449-
}
421+
}
422+
},
423+
{
424+
'name' => msi_2.service_plan.service.name,
425+
'guid' => msi_2.service_plan.service.guid,
426+
'description' => msi_2.service_plan.service.description,
427+
'documentation_url' => 'https://some.url.for.docs/',
428+
'relationships' => {
429+
'service_broker' => {
430+
'data' => {
431+
'guid' => msi_2.service_plan.service.service_broker.guid
450432
}
451433
}
452-
},
453-
{
454-
name: ssi.service_plan.service.name,
455-
guid: ssi.service_plan.service.guid,
456-
description: ssi.service_plan.service.description,
457-
documentation_url: 'https://some.url.for.docs/',
458-
relationships: {
459-
service_broker: {
460-
data: {
461-
guid: ssi.service_plan.service.service_broker.guid
462-
}
434+
}
435+
},
436+
{
437+
'name' => ssi.service_plan.service.name,
438+
'guid' => ssi.service_plan.service.guid,
439+
'description' => ssi.service_plan.service.description,
440+
'documentation_url' => 'https://some.url.for.docs/',
441+
'relationships' => {
442+
'service_broker' => {
443+
'data' => {
444+
'guid' => ssi.service_plan.service.service_broker.guid
463445
}
464446
}
465447
}
466-
],
467-
service_brokers: [
468-
{
469-
name: msi_1.service_plan.service.service_broker.name,
470-
guid: msi_1.service_plan.service.service_broker.guid
471-
},
472-
{
473-
name: msi_2.service_plan.service.service_broker.name,
474-
guid: msi_2.service_plan.service.service_broker.guid
475-
},
476-
{
477-
name: ssi.service_plan.service.service_broker.name,
478-
guid: ssi.service_plan.service.service_broker.guid
479-
}
480-
481-
]
482-
}
448+
}
449+
)
483450

484-
expect({ included: parsed_response['included'] }).to match_json_response({ included: })
451+
# Check service_brokers are included with correct fields (order not guaranteed)
452+
expect(parsed_response['included']['service_brokers']).to contain_exactly(
453+
{
454+
'name' => msi_1.service_plan.service.service_broker.name,
455+
'guid' => msi_1.service_plan.service.service_broker.guid
456+
},
457+
{
458+
'name' => msi_2.service_plan.service.service_broker.name,
459+
'guid' => msi_2.service_plan.service.service_broker.guid
460+
},
461+
{
462+
'name' => ssi.service_plan.service.service_broker.name,
463+
'guid' => ssi.service_plan.service.service_broker.guid
464+
}
465+
)
485466
end
486467
end
487468
end
@@ -851,5 +832,4 @@ def check_filtered_instances(*instances)
851832
end
852833
end
853834
end
854-
855835
end

‎spec/request/service_instances_update_spec.rb‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,10 @@
128128
end
129129

130130
describe 'updates that require broker communication' do
131+
# These tests verify broker request context includes org/space annotations
132+
let!(:org_annotation) { VCAP::CloudController::OrganizationAnnotationModel.make(key_prefix: 'pre.fix', key_name: 'foo', value: 'bar', resource_guid: org.guid) }
133+
let!(:space_annotation) { VCAP::CloudController::SpaceAnnotationModel.make(key_prefix: 'pre.fix', key_name: 'baz', value: 'wow', space: space) }
134+
131135
let(:service_offering) { VCAP::CloudController::Service.make }
132136
let(:original_service_plan) do
133137
VCAP::CloudController::ServicePlan.make(
@@ -1276,5 +1280,4 @@
12761280
end
12771281
end
12781282
end
1279-
12801283
end

‎spec/support/shared_contexts/service_instances_context.rb‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,13 @@
33

44
RSpec.shared_context 'service instances setup' do
55
let(:user) { VCAP::CloudController::User.make }
6-
let(:org) { VCAP::CloudController::Organization.make(created_at: Time.now.utc - 1.second) }
7-
let!(:org_annotation) { VCAP::CloudController::OrganizationAnnotationModel.make(key_prefix: 'pre.fix', key_name: 'foo', value: 'bar', resource_guid: org.guid) }
8-
let(:space) { VCAP::CloudController::Space.make(organization: org, created_at: Time.now.utc - 1.second) }
9-
let!(:space_annotation) { VCAP::CloudController::SpaceAnnotationModel.make(key_prefix: 'pre.fix', key_name: 'baz', value: 'wow', space: space) }
6+
let(:space) { VCAP::CloudController::Space.make }
7+
let(:org) { space.organization }
108
let(:another_space) { VCAP::CloudController::Space.make }
9+
10+
# Only create annotations in tests that actually need them
11+
# Most tests don't check annotations, so don't create them by default
12+
1113
let(:parameters_mixed_data_types_as_json_string) do
1214
'{"boolean":true,"string":"a string","int":123,"float":3.14159,"optional":null,"object":{"a":"b"},"array":["c","d"]}'
1315
end

0 commit comments

Comments
 (0)