Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'd really like someone to sanity check my approach for unit testing the summarise() function and mocking its dependencies.

Background

  • Each option has a set of values, which come from the app state
  • Options also each have a config, which are defined in configs.js (potentially a large list, with 'dynamic' data-like variations)

The tests

I'm testing the summarise function. To isolate my tests, I've mocked the option configs. This means I don't have to couple the test to certain option configs, which allows settings to be changed freely.

My tests work, but I feel it has a few issues:

  1. I can't directly spy on a config object, so I've exported a function getOptionConfig() which I can spy on. I feel like it would be cleaner to avoid an API; and just spy on the object if possible.
  2. I feel it'd be much cleaner to pass the _optionConfigs object to summarise() (to avoid mocks completely). However, this would only pass the same issue to any functions which call summarise().
  3. Because I'm forcing a return value in the spies, I'm not testing the parameter that I'm passing to getOptionConfig(). Is this bad?

configs.js

const _optionConfigs = {
  exampleOption: {
    type: 'red',
  },
  exampleOption2: {
    type: 'blue',
  },
  exampleOption3: {
    type: 'red',
    exclude: true,
  },
  exampleOption4: {
    type: 'red',
  },
  // ... the list goes on...
};

export const getOptionConfig(id) {
  return _optionConfigs[id];
}

summarise.js

import { getOptionConfig } from './config';

/**
 * Summarise an option's values
 * @param {string} optionName - an option name, used for referencing its config data
 * @param {array} optionValues - application state. values of a particular option
 * @returns {string} a summary of the option's values
 */
export const summarise(optionName, optionValues) {
  const optionConfig = getOptionConfig(optionName);
  if (optionConfig.exclude) {
    return '';
  }
  if (optionConfig.type === 'red') {
    return optionValues.map(value => value + ' with a dash of red').join(', ');
  } else (optionConfig.type === 'blue') {
    return optionValues.map(value => value + ' with a bit of blue').join(', ');
  }
}

summarise.test.js

// ENV - jasmine

import * as configs from './configs';

describe('summarise', () => {
  it('ignores when excluded', () => {
    spyOn(configs, 'getOptionConfig').and.returnValue({
      exclude: true,
      type: 'blue',
    });
    const summary = summarise('testOption3');
    expect(summary).toBe('');
  });
  it('summarises blue types', () => {
    spyOn(configs, 'getOptionConfig').and.returnValue({
      type: 'blue',
    });
    const summary = summarise('testOption2', [
      'Value 1',
      'Value 2',
    ]);
    expect(summary).toBe('Value 1 with a bit of blue, Value 2 with a bit of blue');
  });
  it('summarises red types', () => {
    spyOn(configs, 'getOptionConfig').and.returnValue({
      type: 'red',
    });
    const summary = summarise('testOption', [
      'Value 1',
      'Value 2',
    ]);
    expect(summary).toBe('Value 1 with a dash of red, Value 2 with a dash of red');
  });
});
share|improve this question

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.