From d4bc52b212f8a5b94f0452a12a3c01782cfbc2f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Mon, 8 Apr 2019 13:17:22 +0200 Subject: [PATCH 1/2] refactor: improve type safety of unit tests in example apps --- .../controllers/todo-list.controller.unit.ts | 44 ++++++------------- .../unit/controllers/todo.controller.unit.ts | 32 +++----------- .../unit/controllers/todo.controller.unit.ts | 33 ++++---------- 3 files changed, 29 insertions(+), 80 deletions(-) diff --git a/examples/todo-list/src/__tests__/unit/controllers/todo-list.controller.unit.ts b/examples/todo-list/src/__tests__/unit/controllers/todo-list.controller.unit.ts index a7f0aa2d4603..1d2d205cb4b0 100644 --- a/examples/todo-list/src/__tests__/unit/controllers/todo-list.controller.unit.ts +++ b/examples/todo-list/src/__tests__/unit/controllers/todo-list.controller.unit.ts @@ -17,22 +17,6 @@ import {givenTodoList} from '../../helpers'; describe('TodoController', () => { let todoListRepo: StubbedInstanceWithSinonAccessor; - /* - ============================================================================= - METHOD STUBS - These handles give us a quick way to fake the response of our repository - without needing to wrangle fake repository objects or manage real ones - in our tests themselves. - ============================================================================= - */ - let create: sinon.SinonStub; - let count: sinon.SinonStub; - let find: sinon.SinonStub; - let updateAll: sinon.SinonStub; - let findById: sinon.SinonStub; - let updateById: sinon.SinonStub; - let deleteById: sinon.SinonStub; - /* ============================================================================= TEST VARIABLES @@ -55,6 +39,7 @@ describe('TodoController', () => { describe('create()', () => { it('creates a TodoList', async () => { + const create = todoListRepo.stubs.create; create.resolves(aTodoListWithId); expect(await controller.create(aTodoList)).to.eql(aTodoListWithId); sinon.assert.calledWith(create, aTodoList); @@ -63,20 +48,23 @@ describe('TodoController', () => { describe('count()', () => { it('returns the number of existing todoLists', async () => { - count.resolves(aListOfTodoLists.length); - expect(await controller.count()).to.eql(aListOfTodoLists.length); + const count = todoListRepo.stubs.count; + count.resolves({count: aListOfTodoLists.length}); + expect(await controller.count()).to.eql({count: aListOfTodoLists.length}); sinon.assert.called(count); }); }); describe('find()', () => { it('returns multiple todos if they exist', async () => { + const find = todoListRepo.stubs.find; find.resolves(aListOfTodoLists); expect(await controller.find()).to.eql(aListOfTodoLists); sinon.assert.called(find); }); it('returns empty list if no todos exist', async () => { + const find = todoListRepo.stubs.find; const expected: TodoList[] = []; find.resolves(expected); expect(await controller.find()).to.eql(expected); @@ -86,15 +74,18 @@ describe('TodoController', () => { describe('updateAll()', () => { it('returns a number of todos updated', async () => { - updateAll.resolves([aChangedTodoList].length); + const updateAll = todoListRepo.stubs.updateAll; + updateAll.resolves({count: [aChangedTodoList].length}); const where = {title: aTodoListWithId.title}; - expect(await controller.updateAll(aTodoListToPatchTo, where)).to.eql(1); + const result = await controller.updateAll(aTodoListToPatchTo, where); + expect(result).to.eql({count: 1}); sinon.assert.calledWith(updateAll, aTodoListToPatchTo, where); }); }); describe('findById()', () => { it('returns a todo if it exists', async () => { + const findById = todoListRepo.stubs.findById; findById.resolves(aTodoListWithId); expect(await controller.findById(aTodoListWithId.id as number)).to.eql( aTodoListWithId, @@ -105,6 +96,7 @@ describe('TodoController', () => { describe('updateById', () => { it('successfully updates existing items', async () => { + const updateById = todoListRepo.stubs.updateById; updateById.resolves(); await controller.updateById( aTodoListWithId.id as number, @@ -120,6 +112,7 @@ describe('TodoController', () => { describe('deleteById', () => { it('successfully deletes existing items', async () => { + const deleteById = todoListRepo.stubs.deleteById; deleteById.resolves(); await controller.deleteById(aTodoListWithId.id as number); sinon.assert.calledWith(deleteById, aTodoListWithId.id); @@ -147,17 +140,6 @@ describe('TodoController', () => { title: aTodoListToPatchTo.title, }); - // Setup CRUD fakes - ({ - create, - count, - find, - updateAll, - findById, - updateById, - deleteById, - } = todoListRepo.stubs); - controller = new TodoListController(todoListRepo); } }); diff --git a/examples/todo-list/src/__tests__/unit/controllers/todo.controller.unit.ts b/examples/todo-list/src/__tests__/unit/controllers/todo.controller.unit.ts index 51162e65e43c..efba5164b985 100644 --- a/examples/todo-list/src/__tests__/unit/controllers/todo.controller.unit.ts +++ b/examples/todo-list/src/__tests__/unit/controllers/todo.controller.unit.ts @@ -17,21 +17,6 @@ import {givenTodo} from '../../helpers'; describe('TodoController', () => { let todoRepo: StubbedInstanceWithSinonAccessor; - /* - ============================================================================= - METHOD STUBS - These handles give us a quick way to fake the response of our repository - without needing to wrangle fake repository objects or manage real ones - in our tests themselves. - ============================================================================= - */ - let create: sinon.SinonStub; - let findById: sinon.SinonStub; - let find: sinon.SinonStub; - let replaceById: sinon.SinonStub; - let updateById: sinon.SinonStub; - let deleteById: sinon.SinonStub; - /* ============================================================================= TEST VARIABLES @@ -53,6 +38,7 @@ describe('TodoController', () => { describe('createTodo', () => { it('creates a Todo', async () => { + const create = todoRepo.stubs.create; create.resolves(aTodoWithId); const result = await controller.createTodo(aTodo); expect(result).to.eql(aTodoWithId); @@ -62,6 +48,7 @@ describe('TodoController', () => { describe('findTodoById', () => { it('returns a todo if it exists', async () => { + const findById = todoRepo.stubs.findById; findById.resolves(aTodoWithId); expect(await controller.findTodoById(aTodoWithId.id as number)).to.eql( aTodoWithId, @@ -72,12 +59,14 @@ describe('TodoController', () => { describe('findTodos', () => { it('returns multiple todos if they exist', async () => { + const find = todoRepo.stubs.find; find.resolves(aListOfTodos); expect(await controller.findTodos()).to.eql(aListOfTodos); sinon.assert.called(find); }); it('returns empty list if no todos exist', async () => { + const find = todoRepo.stubs.find; const expected: Todo[] = []; find.resolves(expected); expect(await controller.findTodos()).to.eql(expected); @@ -87,6 +76,7 @@ describe('TodoController', () => { describe('replaceTodo', () => { it('successfully replaces existing items', async () => { + const replaceById = todoRepo.stubs.replaceById; replaceById.resolves(); await controller.replaceTodo(aTodoWithId.id as number, aChangedTodo); sinon.assert.calledWith(replaceById, aTodoWithId.id, aChangedTodo); @@ -95,6 +85,7 @@ describe('TodoController', () => { describe('updateTodo', () => { it('successfully updates existing items', async () => { + const updateById = todoRepo.stubs.updateById; updateById.resolves(); await controller.updateTodo(aTodoWithId.id as number, aChangedTodo); sinon.assert.calledWith(updateById, aTodoWithId.id, aChangedTodo); @@ -103,6 +94,7 @@ describe('TodoController', () => { describe('deleteTodo', () => { it('successfully deletes existing items', async () => { + const deleteById = todoRepo.stubs.deleteById; deleteById.resolves(); await controller.deleteTodo(aTodoWithId.id as number); sinon.assert.calledWith(deleteById, aTodoWithId.id); @@ -127,16 +119,6 @@ describe('TodoController', () => { title: 'Do some important things', }); - // Setup CRUD fakes - ({ - create, - findById, - find, - updateById, - replaceById, - deleteById, - } = todoRepo.stubs); - controller = new TodoController(todoRepo); } }); diff --git a/examples/todo/src/__tests__/unit/controllers/todo.controller.unit.ts b/examples/todo/src/__tests__/unit/controllers/todo.controller.unit.ts index 4b08630eb1e0..582ec2d21c25 100644 --- a/examples/todo/src/__tests__/unit/controllers/todo.controller.unit.ts +++ b/examples/todo/src/__tests__/unit/controllers/todo.controller.unit.ts @@ -20,20 +20,6 @@ describe('TodoController', () => { let todoRepo: StubbedInstanceWithSinonAccessor; let geoService: GeocoderService; - /* - ============================================================================= - METHOD STUBS - These handles give us a quick way to fake the response of our repository - without needing to wrangle fake repository objects or manage real ones - in our tests themselves. - ============================================================================= - */ - let create: sinon.SinonStub; - let findById: sinon.SinonStub; - let find: sinon.SinonStub; - let replaceById: sinon.SinonStub; - let updateById: sinon.SinonStub; - let deleteById: sinon.SinonStub; let geocode: sinon.SinonStub; /* @@ -57,6 +43,7 @@ describe('TodoController', () => { describe('createTodo', () => { it('creates a Todo', async () => { + const create = todoRepo.stubs.create; create.resolves(aTodoWithId); const result = await controller.createTodo(aTodo); expect(result).to.eql(aTodoWithId); @@ -64,6 +51,7 @@ describe('TodoController', () => { }); it('resolves remindAtAddress to a geocode', async () => { + const create = todoRepo.stubs.create; geocode.resolves([aLocation.geopoint]); const input = givenTodo({remindAtAddress: aLocation.address}); @@ -84,6 +72,7 @@ describe('TodoController', () => { describe('findTodoById', () => { it('returns a todo if it exists', async () => { + const findById = todoRepo.stubs.findById; findById.resolves(aTodoWithId); expect(await controller.findTodoById(aTodoWithId.id as number)).to.eql( aTodoWithId, @@ -94,12 +83,14 @@ describe('TodoController', () => { describe('findTodos', () => { it('returns multiple todos if they exist', async () => { + const find = todoRepo.stubs.find; find.resolves(aListOfTodos); expect(await controller.findTodos()).to.eql(aListOfTodos); sinon.assert.called(find); }); it('returns empty list if no todos exist', async () => { + const find = todoRepo.stubs.find; const expected: Todo[] = []; find.resolves(expected); expect(await controller.findTodos()).to.eql(expected); @@ -107,6 +98,7 @@ describe('TodoController', () => { }); it('uses the provided filter', async () => { + const find = todoRepo.stubs.find; const filter: Filter = {where: {isCompleted: false}}; find.resolves(aListOfTodos); @@ -117,6 +109,7 @@ describe('TodoController', () => { describe('replaceTodo', () => { it('successfully replaces existing items', async () => { + const replaceById = todoRepo.stubs.replaceById; replaceById.resolves(); await controller.replaceTodo(aTodoWithId.id as number, aChangedTodo); sinon.assert.calledWith(replaceById, aTodoWithId.id, aChangedTodo); @@ -125,6 +118,7 @@ describe('TodoController', () => { describe('updateTodo', () => { it('successfully updates existing items', async () => { + const updateById = todoRepo.stubs.updateById; updateById.resolves(); await controller.updateTodo(aTodoWithId.id as number, aChangedTodo); sinon.assert.calledWith(updateById, aTodoWithId.id, aChangedTodo); @@ -133,6 +127,7 @@ describe('TodoController', () => { describe('deleteTodo', () => { it('successfully deletes existing items', async () => { + const deleteById = todoRepo.stubs.deleteById; deleteById.resolves(); await controller.deleteTodo(aTodoWithId.id as number); sinon.assert.calledWith(deleteById, aTodoWithId.id); @@ -157,16 +152,6 @@ describe('TodoController', () => { title: 'Do some important things', }); - // Setup CRUD fakes - ({ - create, - findById, - find, - updateById, - replaceById, - deleteById, - } = todoRepo.stubs); - geoService = {geocode: sinon.stub()}; geocode = geoService.geocode as sinon.SinonStub; From 2f64f89cca733b76e7e5664636fa3225a5fd3a44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Mon, 8 Apr 2019 13:36:01 +0200 Subject: [PATCH 2/2] feat(metadata): support symbols used as property/method keys --- packages/metadata/src/decorator-factory.ts | 37 +++++++++++++--------- packages/metadata/src/reflect.ts | 23 ++++++++++++-- 2 files changed, 42 insertions(+), 18 deletions(-) diff --git a/packages/metadata/src/decorator-factory.ts b/packages/metadata/src/decorator-factory.ts index 019238267f87..4b4b9c488003 100644 --- a/packages/metadata/src/decorator-factory.ts +++ b/packages/metadata/src/decorator-factory.ts @@ -3,10 +3,10 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT -import {Reflector} from './reflect'; -import * as _ from 'lodash'; import * as debugModule from 'debug'; -import {MetadataMap, DecoratorType, MetadataKey} from './types'; +import * as _ from 'lodash'; +import {Reflector} from './reflect'; +import {DecoratorType, MetadataKey, MetadataMap} from './types'; const debug = debugModule('loopback:metadata:decorator'); // tslint:disable:no-any @@ -124,7 +124,7 @@ export class DecoratorFactory< */ static getTargetName( target: Object, - member?: string, + member?: string | symbol, descriptorOrIndex?: TypedPropertyDescriptor | number, ) { let name = @@ -135,13 +135,17 @@ export class DecoratorFactory< return `class ${name}`; } if (member == null) member = 'constructor'; + + const memberAccessor = + typeof member === 'symbol' ? '[' + member.toString() + ']' : '.' + member; + if (typeof descriptorOrIndex === 'number') { // Parameter - name = `${name}.${member}[${descriptorOrIndex}]`; + name = `${name}${memberAccessor}[${descriptorOrIndex}]`; } else if (descriptorOrIndex != null) { - name = `${name}.${member}()`; + name = `${name}${memberAccessor}()`; } else { - name = `${name}.${member}`; + name = `${name}${memberAccessor}`; } return name; } @@ -210,7 +214,7 @@ export class DecoratorFactory< protected mergeWithInherited( inheritedMetadata: M, target: Object, - member?: string, + member?: string | symbol, descriptorOrIndex?: TypedPropertyDescriptor | number, ): M { throw new Error('mergeWithInherited() is not implemented'); @@ -232,7 +236,7 @@ export class DecoratorFactory< protected mergeWithOwn( ownMetadata: M, target: Object, - member?: string, + member?: string | symbol, descriptorOrIndex?: TypedPropertyDescriptor | number, ): M { throw new Error('mergeWithOwn() is not implemented'); @@ -254,7 +258,7 @@ export class DecoratorFactory< */ protected decorate( target: Object, - member?: string, + member?: string | symbol, descriptorOrIndex?: TypedPropertyDescriptor | number, ) { const targetName = DecoratorFactory.getTargetName( @@ -430,7 +434,7 @@ export class PropertyDecoratorFactory extends DecoratorFactory< } create(): PropertyDecorator { - return (target: Object, propertyName: string) => + return (target: Object, propertyName: string | symbol) => this.decorate(target, propertyName); } @@ -498,7 +502,7 @@ export class MethodDecoratorFactory extends DecoratorFactory< create(): MethodDecorator { return ( target: Object, - methodName: string, + methodName: string | symbol, descriptor: TypedPropertyDescriptor, ) => this.decorate(target, methodName, descriptor); } @@ -592,8 +596,11 @@ export class ParameterDecoratorFactory extends DecoratorFactory< } create(): ParameterDecorator { - return (target: Object, methodName: string, parameterIndex: number) => - this.decorate(target, methodName, parameterIndex); + return ( + target: Object, + methodName: string | symbol, + parameterIndex: number, + ) => this.decorate(target, methodName, parameterIndex); } /** @@ -723,7 +730,7 @@ export class MethodParameterDecoratorFactory extends DecoratorFactory< create(): MethodDecorator { return ( target: Object, - methodName: string, + methodName: string | symbol, descriptor: TypedPropertyDescriptor, ) => this.decorate(target, methodName, descriptor); } diff --git a/packages/metadata/src/reflect.ts b/packages/metadata/src/reflect.ts index d7baf94dcd16..2c7c56ce1b4c 100644 --- a/packages/metadata/src/reflect.ts +++ b/packages/metadata/src/reflect.ts @@ -148,14 +148,31 @@ export class NamespacedReflect { return metaKeys; } + decorate( + decorators: (PropertyDecorator | MethodDecorator)[], + target: Object, + targetKey?: string | symbol, + descriptor?: PropertyDescriptor, + ): PropertyDescriptor | Function; + + decorate( + decorators: ClassDecorator[], + target: Object, + ): PropertyDescriptor | Function; + decorate( decorators: (PropertyDecorator | MethodDecorator)[] | ClassDecorator[], target: Object, - targetKey?: string, + targetKey?: string | symbol, descriptor?: PropertyDescriptor, ): PropertyDescriptor | Function { if (targetKey) { - return Reflect.decorate(decorators, target, targetKey, descriptor); + return Reflect.decorate( + <(PropertyDecorator | MethodDecorator)[]>decorators, + target, + targetKey, + descriptor, + ); } else { return Reflect.decorate(decorators, target); } @@ -167,7 +184,7 @@ export class NamespacedReflect { metadataValue: any, ): { (target: Function): void; - (target: Object, targetKey: string): void; + (target: Object, targetKey: string | symbol): void; } { metadataKey = this.getMetadataKey(metadataKey); return Reflect.metadata(metadataKey, metadataValue);