4

I'm working on an enterprise that has some Angular/Typescript projects and to avoid repeating code (basically copying and pasting) between them, we decided to go for Monorepo and start write an util library, with unit tests, docs and everything on.

At the moment we're implementing an util function:

export const normalizeNames = (value: string): string => {
  if (!isString(value)) {
    // throw some error
  }

  // ...
}

Just as the company is relatively new to testing concepts in general, so am I.

Since we are at an impasse trying to establish a standard of how the tests should be structured and what we should test, I decided to open this question here.

The first thing that came to my mind was to separate them in two main groups:

  • Invalid -> A test for each invalid type I could imagine, like: 1 null, 1 undefined, 1 NaN, 1 boolean, 1 number, 1 array, and others like Buffer, Map, Object, RegExp, Set, etc.;
  • Valid;

... something like this:

describe('normalizeNames', () => {
  describe('invalid', () => {
    it(`should throw error for the value 'null'`, () => {
      expect(() => normalizeNames(null as any)).toThrowError(
        TypeError,
      );
    });

    it(`should throw error for the value 'undefined'`, () => {
      expect(() => normalizeNames(undefined as any)).toThrowError(
        TypeError,
      );
    });

    // other types
  });

  describe('valid', () => {
    it(`should return '' for the value ''`, () => {
      expect(normalizeNames('')).toBe('');
    });

    it(`should return 'Stack' for the value 'stack'`, () => {
      expect(normalizeNames('stack')).toBe('Stack');
    });

    // ... more tests
  });
});

...but then I noticed that if I test all the types I can imagine, the tests would be too big and maybe difficult to maintain.

Another solution that I thought is to create two Arrays and do something like below, to avoid the repetition:

const invalidTestCases = [
  { actual: null, expected: TypeError },
  { actual: undefined, expected: TypeError },
  // more...
];
const validTestCases = [
  { actual: '', expected: '' },
  { actual: 'stack', expected: 'Stack' }, // it's just a sample data
  // more...
];

describe('normalizeNames', () => {
  describe('invalid', () => {
    for (const { actual, expected } of invalidTestCases) {
      it(`should throw error for the value '${actual}'`, () => {
        expect(() => normalizeNames(actual as any)).toThrowError(
          expected,
        );
      });
    }
  });

  describe('valid', () => {
    for (const { actual, expected } of validTestCases) {
      it(`should return '${expected}' for the value '${actual}'`, () => {
        expect(() => normalizeNames(actual as any)).toBe(expected);
      });
    }
  });
});

So, the questions are basically:

  1. Is it okay to separate the tests in these two main "groups"?
  2. Is it acceptable to have tests for all possible "types"? Otherwise, which entries would you recommend for invalid tests?
  3. For the second solution: is it a good practice to write tests in that way, with loops?
4
  • 5
    Loops in tests are ok, but if your testing framework supports parametrized tests or data driven tests, that will be usually better.
    – Doc Brown
    Apr 12 '20 at 6:14
  • 2
    ... after googling a few minutes, it seems the test framework you are using is Jasmine, which does not support data-driven tests out of the box. But I found this article presenting two workarounds for this issue.
    – Doc Brown
    Apr 12 '20 at 6:22
  • surely typescript wont compile if you pass a non string
    – Ewan
    Apr 12 '20 at 8:32
  • 2
    Hey @DocBrown thanks for your comments. It really helped. I'm using Jest and I found that Jest has test.each/it.each and I'm going to use it right now.
    – dev_054
    Apr 12 '20 at 14:38
6

As a general rule, whatever conventions your team comes up with and agrees upon is good. Just be consistent in your project.

I have worked on teams that use exactly the conventions you are describing, and it has worked well for us.

To give some detail to each of your questions:

Is it okay to separate the tests in these two main "groups"?

yes! describe blocks are there to group tests to make them easy to read. This of them like "headers" in an spec. Your tests are the documentation for your code. Group the tests so that a future reader can "scan" the describe blocks to find the tests they care about looking at.

Is it acceptable to have tests for all possible "types"?

Of course! If you think it needs tested, you should test it. If you think it doesn't need to be tested, you may want to test it anyway. Unit tests run incredibly fast, and as long as you keep your test organized well, I have pretty much never heard people complain about having "too many unit tests".

is it a good practice to write tests with loops?

Sure! Your test code should be treated the same as your "production" code. Any principles you think matter for production code, also matters for test code. So tools to reduce duplication, and keep things organized and easy to read is a great idea.

One thing that we have done on teams in past to make this even easier is to add the "description" of the test case into the object you are passing in the loop. This help future readers see why the different cases matter. And we try to keep our "test cases" close to the it block so that future readers don't have to scroll around a lot.

Something like this:

describe('normalizeNames', () => {   
    ... // other tests

    describe('valid input', () => {
        [
            {
                description: 'empty strings normalize as empty string',
                input: '',
                expected: ''
            },
            {
                description: 'names with hyphens are treated as a single word',
                input: 'sOme-Named-pErSon',
                expected: 'Some-named-person'
            },
            {
                description: 'names with spaces are treated as multiple words',
                input: 'some person name',
                expected: 'Some Person Name'
            },
            // other test cases for your business logic...
        ].forEach({ input, expected, description }) {
            it(`can normalizeNames for valid input - ${description}.  input: '${input}', expected: '${expected}'`, () => {
                expect(() => normalizeNames(actual)).toBe(expected);
            });
        }   
    }); 
});
1
  • Thanks a lot for your answer. It clarified things a little more :)
    – dev_054
    Apr 12 '20 at 14:39
3

Generally you want tests to test one single thing and have a name that describes what they are testing.

Of you put a loop in your test to make it test lots of things you can run into problems.

  1. does it stop when it hit the first error?
  2. when it errors does it tell you which case failed?
  3. Has it become complex enough that you might have an error in your tests

As @doc mentions in their comment, the way around this is data driven tests.

Data driven tests allow you to use the same code for multiple tests, generating a new name in the test output and using different input for each.

[TestCase(null)]
[TestCase(MaxInt)]
[etc]
InputShouldBeValid(input:any)
{
      ...test and assert
}
3

Adding on to the existing answers here -

Instead of enumerating all different cases for your tests, think about the properties of your code that will always be true regardless of the input.

After that, apply a property-based testing framework to help you perform the value generation + assertion. More details here: https://marmelab.com/blog/2019/04/18/property-based-testing-js.html

A Property Based Testing using library such as fast-check is an interesting alternative to exhaustive enumeration. It allows to find bugs both in the implementation code and in the specification of the property we want to check. In both cases this helps the developer to get confident in the product he is developing.

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.