Service Example for a testing question

tasks.service.ts

switchNodeParents(taskNode: TaskNode, newParentTaskNode: TaskNode, toIndex: PutAfterThisNode | number | 'last' = 'last') {
  this.removeNodeFromCurrentParent(taskNode);
  this.addTaskNode(taskNode, newParentTaskNode, toIndex);
}

createAndAddTaskNode(task: Task, toParentNode: TaskNode = null, toIndex: PutAfterThisNode | 'last' | number = 0): TaskNode {
  this.tasksByIds.set(task.id, task);
  let taskNode = new TaskNode({ task });
  this.addTaskNode(taskNode, toParentNode, toIndex);
  return taskNode;
}

addTaskNode(taskNode: TaskNode, toParentNode: TaskNode, toIndex: PutAfterThisNode | 'last' | number = 0) {
  let task = taskNode.task;
  let category = task.category;
  if (!category) {
    throw new Error('Category on new item must be defined');
  }
  let categoryNodes = this.taskNodesByCategory.get(category);

  if (toIndex !== 0 && toIndex >= categoryNodes.length) {
    throw new RangeError('toIndex is out of range');
  }

  let taskNodesToJoin = categoryNodes;

  if (toParentNode) {
    taskNodesToJoin = toParentNode.children;
    taskNode.parent = toParentNode;
    task.parentId = toParentNode.task.id;
    toParentNode.task.childrenIds.push(task.id);
    toParentNode.task.opened = true;
    this.saveTaskDetails(toParentNode.task);
  }

  if (toIndex instanceof PutAfterThisNode) {
    let indexOfTargetItem = taskNodesToJoin.indexOf(toIndex.nodeToPutAfter);
    toIndex = (toIndex.location === 'before') ? indexOfTargetItem : indexOfTargetItem + 1;
  }

  if (toIndex === 'last') {
    toIndex = taskNodesToJoin.length;
  }

  task.order = toIndex;

  taskNodesToJoin.splice(toIndex, 0, taskNode);

  this.saveTaskDetails(task);

  this.reorderFromThisIndex(taskNodesToJoin, toIndex);
}

Regarding the issue of spying on part of one object to check another part, I generally prefer not to do that. In this case, however, these three functions are not really methods of an object, but merely functions in a shared namespace. They neither read nor write to any fields in common. (There is no field on this that two of these functions both touch.) We could easily split them into three different objects and these functions wouldn't change in any particularly interesting way. They already act as though they are on separate objects, so spying on addTaskNode() in order to check switchNodeParents() and createAndAddTaskNode() doesn't worry me.

If we introduce this.tasksByIds as a parameter to createAndAddTaskNode() and do the same for this.taskNodesByCategory in addTaskNode(), then all the typical concerns about spying on one part of an object to check another go away.

The risk tends to relate to writing tests that need to simulate implementation details in a manner than replicates some as-needed subset of the contract of the object. If we want to write tests that depend on the contract of an object, then let's change the code so that the object we're checking depends directly on that contract--abstraction/protocol/interface--through an explicit parameter.

In this case, I'd just spy on addTaskNode() and look for more signals that it's worth moving that function onto an abstraction separate from the object(s) that implement(s) its two clients.