From 5f70fc415bfde65dcc2c9832b2b680cefcfb39f7 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Wed, 19 Oct 2022 10:28:40 -0700 Subject: [PATCH] chore: update procedure map tests to match the refactored API (#6562) * fix: feedback on procedure model implementations * chore: format * chore: add tests for the backing variable of parameter models * chore: update existing procedure map tests * chore: update block update tests to use refactored API * chore: update tests to actually use fluent API * chore: format * chore: fix tests * chore: reorganize tests * chore: format * chore: add comment --- core/blockly.ts | 1 + core/procedures.ts | 9 + core/procedures/observable_parameter_model.ts | 10 +- core/procedures/observable_procedure_map.ts | 9 +- core/procedures/observable_procedure_model.ts | 5 + tests/mocha/procedure_map_test.js | 155 +++++++++++++----- 6 files changed, 139 insertions(+), 50 deletions(-) diff --git a/core/blockly.ts b/core/blockly.ts index 71c19f77d..41946d4d4 100644 --- a/core/blockly.ts +++ b/core/blockly.ts @@ -602,6 +602,7 @@ export {Css}; export {Events}; export {Extensions}; export {Procedures}; +export {Procedures as procedures}; export {ShortcutItems}; export {Themes}; export {Tooltip}; diff --git a/core/procedures.ts b/core/procedures.ts index 7fad925e6..e31d77d31 100644 --- a/core/procedures.ts +++ b/core/procedures.ts @@ -25,6 +25,9 @@ import * as eventUtils from './events/utils.js'; import {Field, UnattachedFieldError} from './field.js'; import {Msg} from './msg.js'; import {Names} from './names.js'; +import {ObservableProcedureMap} from './procedures/observable_procedure_map.js'; +import {ObservableProcedureModel} from './procedures/observable_procedure_model.js'; +import {ObservableParameterModel} from './procedures/observable_parameter_model.js'; import * as utilsXml from './utils/xml.js'; import * as Variables from './variables.js'; import type {Workspace} from './workspace.js'; @@ -450,3 +453,9 @@ export function getDefinition(name: string, workspace: Workspace): Block|null { } return null; } + +export { + ObservableProcedureMap, + ObservableProcedureModel, + ObservableParameterModel, +}; diff --git a/core/procedures/observable_parameter_model.ts b/core/procedures/observable_parameter_model.ts index 4c74a0505..407b0ad6d 100644 --- a/core/procedures/observable_parameter_model.ts +++ b/core/procedures/observable_parameter_model.ts @@ -17,13 +17,15 @@ export class ObservableParameterModel implements IParameterModel { constructor( private readonly workspace: Workspace, name: string, id?: string) { this.id = id ?? genUid(); - this.variable = workspace.createVariable(name); + this.variable = + this.workspace.getVariable(name) ?? workspace.createVariable(name); } /** * Sets the name of this parameter to the given name. */ setName(name: string): this { + // TODO(#6516): Fire events. if (name == this.variable.name) return this; this.variable = this.workspace.getVariable(name) ?? this.workspace.createVariable(name); @@ -34,12 +36,14 @@ export class ObservableParameterModel implements IParameterModel { * Unimplemented. The built-in ParameterModel does not support typing. * If you want your procedure blocks to have typed parameters, you need to * implement your own ParameterModel. + * + * @throws Throws for the ObservableParameterModel specifically because this + * method is unimplemented. */ setTypes(_types: string[]): this { - console.warn( + throw new Error( 'The built-in ParameterModel does not support typing. You need to ' + 'implement your own custom ParameterModel.'); - return this; } /** diff --git a/core/procedures/observable_procedure_map.ts b/core/procedures/observable_procedure_map.ts index db5edfe68..35ee4a5e7 100644 --- a/core/procedures/observable_procedure_map.ts +++ b/core/procedures/observable_procedure_map.ts @@ -17,7 +17,7 @@ export class ObservableProcedureMap extends Map { * Adds the given procedure model to the procedure map. */ override set(id: string, proc: IProcedureModel): this { - // TODO(#6156): Fire events. + // TODO(#6516): Fire events. super.set(id, proc); return this; } @@ -27,7 +27,7 @@ export class ObservableProcedureMap extends Map { * exists). */ override delete(id: string): boolean { - // TODO(#6156): Fire events. + // TODO(#6516): Fire events. return super.delete(id); } @@ -35,7 +35,7 @@ export class ObservableProcedureMap extends Map { * Removes all ProcedureModels from the procedure map. */ override clear() { - // TODO(#6156): Fire events. + // TODO(#6516): Fire events. super.clear(); } @@ -44,7 +44,8 @@ export class ObservableProcedureMap extends Map { * blocks can find it. */ add(proc: IProcedureModel): this { - // TODO(#6156): Fire events. + // TODO(#6516): Fire events. + // TODO(#6526): See if this method is actually useful. return this.set(proc.getId(), proc); } } diff --git a/core/procedures/observable_procedure_model.ts b/core/procedures/observable_procedure_model.ts index 335667ab9..aa51bb7f9 100644 --- a/core/procedures/observable_procedure_model.ts +++ b/core/procedures/observable_procedure_model.ts @@ -23,6 +23,7 @@ export class ObservableProcedureModel implements IProcedureModel { /** Sets the human-readable name of the procedure. */ setName(name: string): this { + // TODO(#6516): Fire events. this.name = name; return this; } @@ -33,12 +34,14 @@ export class ObservableProcedureModel implements IProcedureModel { * To move a parameter, first delete it, and then re-insert. */ insertParameter(parameterModel: IParameterModel, index: number): this { + // TODO(#6516): Fire events. this.parameters.splice(index, 0, parameterModel); return this; } /** Removes the parameter at the given index from the parameter list. */ deleteParameter(index: number): this { + // TODO(#6516): Fire events. this.parameters.splice(index, 1); return this; } @@ -49,6 +52,7 @@ export class ObservableProcedureModel implements IProcedureModel { * Pass null to represent a procedure that does not return. */ setReturnTypes(types: string[]|null): this { + // TODO(#6516): Fire events. this.returnTypes = types; return this; } @@ -58,6 +62,7 @@ export class ObservableProcedureModel implements IProcedureModel { * all procedure caller blocks should be disabled as well. */ setEnabled(enabled: boolean): this { + // TODO(#6516): Fire events. this.enabled = enabled; return this; } diff --git a/tests/mocha/procedure_map_test.js b/tests/mocha/procedure_map_test.js index 08e5f8137..60a84ad37 100644 --- a/tests/mocha/procedure_map_test.js +++ b/tests/mocha/procedure_map_test.js @@ -8,18 +8,19 @@ import {sharedTestSetup, sharedTestTeardown} from './test_helpers/setup_teardown goog.declareModuleId('Blockly.test.procedureMap'); -suite.skip('Procedure Map', function() { +suite('Procedure Map', function() { setup(function() { sharedTestSetup.call(this); this.workspace = new Blockly.Workspace(); - this.procedureMap = this.workspace.getProcedureMap(); + // this.procedureMap = this.workspace.getProcedureMap(); }); teardown(function() { sharedTestTeardown.call(this); }); - suite('triggering block updates', function() { + // TODO (#6515): Unskip tests. + suite.skip('triggering block updates', function() { setup(function() { Blockly.Blocks['procedure_mock'] = { init: function() { }, @@ -36,8 +37,17 @@ suite.skip('Procedure Map', function() { }); suite('procedure map updates', function() { + test('inserting a procedure does not trigger an update', function() { + const procedureModel = + new Blockly.procedures.ObservableProcedureModel(this.workspace); + this.procedureMap.set(procedureModel.getId(), procedureModel); + + chai.assert.isFalse( + this.updateSpy.called, 'Expected no update to be triggered'); + }); + test('adding a procedure does not trigger an update', function() { - this.procedureMap.addProcedure( + this.procedureMap.add( new Blockly.procedures.ObservableProcedureModel(this.workspace)); chai.assert.isFalse( @@ -47,9 +57,9 @@ suite.skip('Procedure Map', function() { test('deleting a procedure triggers an update', function() { const procedureModel = new Blockly.procedures.ObservableProcedureModel(this.workspace); - this.procedureMap.addProcedure(procedureModel); + this.procedureMap.add(procedureModel); - this.procedureMap.deleteProcedure(procedureModel.getId()); + this.procedureMap.delete(procedureModel.getId()); chai.assert.isTrue( this.updateSpy.calledOnce, 'Expected an update to be triggered'); @@ -60,7 +70,7 @@ suite.skip('Procedure Map', function() { test('setting the name triggers an update', function() { const procedureModel = new Blockly.procedures.ObservableProcedureModel(this.workspace); - this.procedureMap.addProcedure(procedureModel); + this.procedureMap.add(procedureModel); procedureModel.setName('new name'); @@ -71,9 +81,9 @@ suite.skip('Procedure Map', function() { test('setting the return type triggers an update', function() { const procedureModel = new Blockly.procedures.ObservableProcedureModel(this.workspace); - this.procedureMap.addProcedure(procedureModel); + this.procedureMap.add(procedureModel); - procedureModel.setReturnType(['return type 1', 'return type 2']); + procedureModel.setReturnTypes(['return type 1', 'return type 2']); chai.assert.isTrue( this.updateSpy.calledOnce, 'Expected an update to be triggered'); @@ -81,12 +91,12 @@ suite.skip('Procedure Map', function() { test('removing the return type triggers an update', function() { const procedureModel = - new Blockly.procedures.ObservableProcedureModel(this.workspace); - this.procedureMap.addProcedure(procedureModel); + new Blockly.procedures.ObservableProcedureModel(this.workspace) + .setReturnTypes(['return type']); + this.procedureMap.add(procedureModel); + this.updateSpy.resetHistory(); - procedureModel.setReturnType(['return type']); - this.updateSpy.reset(); - procedureModel.setReturnType(null); + procedureModel.setReturnTypes(null); chai.assert.isTrue( this.updateSpy.calledOnce, 'Expected an update to be triggered'); @@ -95,7 +105,7 @@ suite.skip('Procedure Map', function() { test('disabling the procedure triggers an update', function() { const procedureModel = new Blockly.procedures.ObservableProcedureModel(this.workspace); - this.procedureMap.addProcedure(procedureModel); + this.procedureMap.add(procedureModel); procedureModel.setEnabled(false); @@ -105,10 +115,10 @@ suite.skip('Procedure Map', function() { test('enabling the procedure triggers an update', function() { const procedureModel = - new Blockly.procedures.ObservableProcedureModel(this.workspace); - this.procedureMap.addProcedure(procedureModel); - procedureModel.setEnabled(false); - this.updateSpy.reset(); + new Blockly.procedures.ObservableProcedureModel(this.workspace) + .setEnabled(false); + this.procedureMap.add(procedureModel); + this.updateSpy.resetHistory(); procedureModel.setEnabled(true); @@ -119,7 +129,7 @@ suite.skip('Procedure Map', function() { test('inserting a parameter triggers an update', function() { const procedureModel = new Blockly.procedures.ObservableProcedureModel(this.workspace); - this.procedureMap.addProcedure(procedureModel); + this.procedureMap.add(procedureModel); procedureModel.insertParameter( new Blockly.procedures.ObservableParameterModel(this.workspace)); @@ -130,11 +140,12 @@ suite.skip('Procedure Map', function() { test('deleting a parameter triggers an update', function() { const procedureModel = - new Blockly.procedures.ObservableProcedureModel(this.workspace); - this.procedureMap.addProcedure(procedureModel); - procedureModel.insertParameter( - new Blockly.procedures.ObservableParameterModel(this.workspace)); - this.updateSpy.reset(); + new Blockly.procedures.ObservableProcedureModel(this.workspace) + .insertParameter( + new Blockly.procedures.ObservableParameterModel( + this.workspace)); + this.procedureMap.add(procedureModel); + this.updateSpy.resetHistory(); procedureModel.deleteParameter(0); @@ -144,34 +155,31 @@ suite.skip('Procedure Map', function() { }); suite('parameter model updates', function() { - test('setting the variable model triggers an update', function() { - const procedureModel = - new Blockly.procedures.ObservableProcedureModel(this.workspace); - this.procedureMap.addProcedure(procedureModel); + test('setting the name triggers an update', function() { const parameterModel = - new Blockly.procedures.ObservableParameterModel(this.workspace); - procedureModel.insertParameter(parameterModel); - this.updateSpy.reset(); + new Blockly.procedures.ObservableParameterModel( + this.workspace, 'test1'); + this.procedureMap.add( + new Blockly.procedures.ObservableProcedureModel(this.workspace) + .insertParameter(parameterModel)); + this.updateSpy.resetHistory(); - parameterModel.setVariable( - new Blockly.VariableModel(this.workspace, 'variable')); + parameterModel.setName('test2'); chai.assert.isTrue( this.updateSpy.calledOnce, 'Expected an update to be triggered'); }); test('modifying the variable model does not trigger an update', function() { - const procedureModel = - new Blockly.procedures.ObservableProcedureModel(this.workspace); - this.procedureMap.addProcedure(procedureModel); const parameterModel = - new Blockly.procedures.ObservableParameterModel(this.workspace); - procedureModel.insertParameter(parameterModel); - const variableModel = - new Blockly.VariableModel(this.workspace, 'variable'); - parameterModel.setVariable(variableModel); - this.updateSpy.reset(); + new Blockly.procedures.ObservableParameterModel( + this.workspace, 'test1'); + this.procedureMap.add( + new Blockly.procedures.ObservableProcedureModel(this.workspace) + .insertParameter(parameterModel)); + this.updateSpy.resetHistory(); + const variableModel = parameterModel.getVariableModel(); variableModel.name = 'some name'; variableModel.type = 'some type'; @@ -184,4 +192,65 @@ suite.skip('Procedure Map', function() { suite('event firing', function() { // TBA after the procedure map is implemented }); + + suite('backing variable to parameters', function() { + test( + 'construction references an existing variable if available', + function() { + const variable = this.workspace.createVariable('test1'); + const param = new Blockly.procedures.ObservableParameterModel( + this.workspace, 'test1'); + + chai.assert.equal( + variable, + param.getVariableModel(), + 'Expected the parameter model to reference the existing variable'); + }); + + test('construction creates a variable if none exists', function() { + const param = new Blockly.procedures.ObservableParameterModel( + this.workspace, 'test1'); + + chai.assert.equal( + this.workspace.getVariable('test1'), + param.getVariableModel(), + 'Expected the parameter model to create a variable'); + }); + + test('setName references an existing variable if available', function() { + const variable = this.workspace.createVariable('test2'); + const param = new Blockly.procedures.ObservableParameterModel( + this.workspace, 'test1'); + + param.setName('test2'); + + chai.assert.equal( + variable, + param.getVariableModel(), + 'Expected the parameter model to reference the existing variable'); + }); + + test('setName creates a variable if none exits', function() { + const param = new Blockly.procedures.ObservableParameterModel( + this.workspace, 'test1'); + + param.setName('test2'); + + chai.assert.equal( + this.workspace.getVariable('test2'), + param.getVariableModel(), + 'Expected the parameter model to create a variable'); + }); + + test('setTypes is unimplemented', function() { + const param = new Blockly.procedures.ObservableParameterModel( + this.workspace, 'test1'); + + chai.assert.throws( + () => { + param.setTypes(['some', 'types']); + }, + 'The built-in ParameterModel does not support typing'); + }); + }); });