4
\$\begingroup\$

Instead Of Intro

I am a .NET developer who has recently moved to front-end realms. Unit testing was never a strong side of mine, but what I learnt is that the test code has to be of highest quality possible (follow DRY, SOLID, etc). Otherwise, the code faces rapidly increasing risk of rotting since test maintenance becomes burdensome.

Not having experience with JavaScript/Jasmine makes me write C#-ish code in real system and tests. Getting some feedback should help in long run with keeping project in a good shape.

Code

Class under test (Spectra) declares several methods which are made public for now. Each method is essentially a pure function, so it's really easy to test each of them independently.

export class Spectra {
  public static numberInRangeToRgb(value: number, min: number, max: number): string { ... }
  public static waveLengthFromDataPoint(value: number, minValue: number, maxValue: number): number { ... }
  public static waveLengthToRGB(waveLengthInNanoMeters: number): number[] { ... }
  public static rgbFromWaveLength(waveLengthInNanoMeters: number): number[] { ... }
  public static factorFromWaveLength(waveLengthInNanoMeters: number): number { ... }
  public static adjust(color: number, factor: number): number { ... }
}

Here are the unit tests I've written so far.

describe('Spectra', () => {

  function title(testCase: any): string {
    return `works for ${JSON.stringify(testCase)} test case`;
  }

  describe('numberInRangeToRgb', () => {
    [
      { value: 0, min: 0, max: 10, expectedResult: '#000000' },
      { value: 5, min: 0, max: 10, expectedResult: '#00ff92' },
      { value: 10, min: 0, max: 10, expectedResult: '#ff0000' },
      { value: 0, min: -10, max: 0, expectedResult: '#ff0000' },
      // More cases
    ].forEach(testCase => {
      it(title(testCase), () => {
        const { value, min, max, expectedResult } = testCase;
        expect(Spectra.numberInRangeToRgb(value, min, max)).toEqual(expectedResult);
      });
    });
  });

  describe('waveLengthFromDataPoint', () => {
    [
      { value: 10, min: 10, max: 20, expectedResult: 350 },
      // More cases
    ].forEach(testCase => {
      it(title(testCase), () => {
        const { value, min, max, expectedResult } = testCase;
        expect(Spectra.waveLengthFromDataPoint(value, min, max)).toBeCloseTo(expectedResult);
      });
    });
  });

  describe('waveLengthToRGB', () => {
    [
      { waveLength: 300, expectedResult: [0, 0, 0] },
      // More cases
    ].forEach(testCase => {
      it(title(testCase), () => {
        const { waveLength, expectedResult } = testCase;
        expect(Spectra.waveLengthToRGB(waveLength)).toEqual(expectedResult);
      });
    });
  });

  describe('rgbFromWaveLength', () => {
    [
      { waveLength: -100, expectedResult: [0, 0, 0] },
      // More cases
    ].forEach(testCase => {
      it(title(testCase), () => {
        const { waveLength, expectedResult } = testCase;
        expect(Spectra.rgbFromWaveLength(waveLength)).toEqual(expectedResult);
      });
    });
  });

  describe('factorFromWaveLength', () => {
    [
      { waveLength: -100, expectedFactor: 0 },
      // More cases
    ].forEach(testCase => {
      it(title(testCase), () => {
        const { waveLength, expectedFactor } = testCase;
        expect(Spectra.factorFromWaveLength(waveLength)).toBeCloseTo(expectedFactor);
      });
    });
  });

  describe('adjust', () => {
    [
      { color: 1, factor: 2, expectedResult: 444 },
      // More cases
    ].forEach(testCase => {
      it(title(testCase), () => {
        const { color, factor, expectedResult } = testCase;
        expect(Spectra.adjust(color, factor)).toBe(expectedResult);
      });
    });
  });

});

Code Review Focus

I would really appreciate constructive feedback on:

  1. Whether I miss anything in terms of JavaScript/TypeScript idioms.
  2. Same about Jasmine idioms, techniques, approaches.
  3. General code improvements.
  4. Test differentiation/separation techniques (some are real unit-tests, other are more of integration tests).

Update 1 - Test Results

This is what Karma test runner will produce as a result of the test suite run, by the way.

enter image description here


One thing I should apply is object destructuring, and this

[
  { value: 0, min: 0, max: 10, expectedResult: '#000000' },
  // More cases
].forEach(testCase => {
  it(title(testCase), () => {
    const { value, min, max, expectedResult } = testCase;
    // Code of the test...
  }
}

will become this

[
  { value: 0, min: 0, max: 10, expectedResult: '#000000' },
  // More cases
].forEach(({ value, min, max, expectedResult}) => {
  it(`...`, () => {
    // Code of the test...
  }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Does the test suite also contain invalid cases? \$\endgroup\$ – Attilio Dec 5 '17 at 19:08
  • \$\begingroup\$ @Attilio No it does not, but it should. Very good point! \$\endgroup\$ – Igor Soloydenko Dec 5 '17 at 19:29
  • 1
    \$\begingroup\$ Idiomatically you should absolutely prefer exporting functions and variables directly over using a class with static methods. \$\endgroup\$ – Aluan Haddad Mar 12 '18 at 9:49
1
\$\begingroup\$

Although this is one way of avoiding repetition, each it clause doesn't really describe what it is doing, just what is expected. Taking your first one for example (describe('numberInRangeToRgb',...) I would rather see:

describe('numberInRangeToRgb', () => {
  it('should return black when the value is at the beginning of the range', ...
  it('should return green when the value is in the range', ...
  it('should return red when the value is at the end of the range', ...
  it('should return red when the value is out of range', ...

I would write this something like:

describe('numberInRangeToRgb', () => {

  function test(value, min, max, expectedResult) { 
    expect(Spectra.numberInRangeToRgb(value, min, max)).toEqual(expectedResult);
  };

  it('should return black when the value is at the beginning of the range', 
     => { test( 0,  0, 10, '#000000'); );         

  // More cases

Another way way to do this might be to add a descriptive title to your hashes:

describe('numberInRangeToRgb', () => {
  [
    { title: 'should return black if the value is at the beginning of the range', 
      value: 0, min: 0, max: 10, expectedResult: '#000000' }, 
    // More cases
  ].forEach(testCase => {
    it(testCase.title), () => {
\$\endgroup\$
  • \$\begingroup\$ test( 0, 0, 10, '#000000') without named parameters is cryptic in my opinion -- what are 0, 0, and 10? doesn't really describe what it is doing, just what is expected -- I agree; the reason I do it this way is because I'm trying to be pragmatic about the time I spend on that. Also, adding too specific titles makes the tests fragile or hard to maintain/keep in sync with the claim (in other words, violation of DRY in a way). I completely agree that its are "imperfect" at the very best, but I don't find the proposed solution adoptable in a short/medium run. \$\endgroup\$ – Igor Soloydenko May 9 '17 at 17:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.