Basic unit tests for stock service

[mbayopanda]

This PR add unit tests for some functions of the stock service and fix some export feature in stock registries.

[jniles]

@mbayopanda, nice tests! Your testing logic is pretty good, but there is a lot of set up which makes the test a bit hard to read. I propose you set up all your data, either in the top of the test file or in a MockService, and then begin executing test logic.

It's also typically better to check against new objects/values instead of the original objects/values when you write a test. Imagine the following function:

const o = {
  id : 1,
  data : { hello : 'world' }
};

function getData(o) {
  return o.data;
}

Now imagine you wrote the following test:

const exampleO = {
  id : 2,
  data : { food : 'sandwich' },
}

const result = getData(exampleO);
expect(results).to.equal(exampleO.data);

This test looks okay, but in reality, it may not catch all the changes I could make to the object. Imagine that I begin refactoring and change the function getData to be:

function getData(o) {
  delete o.data;
  return undefined;
}

I'm pretty sure the tests above would still pass, even though the behavior is completely different! That is because delete will remove o.data, which is equal to undefined. I wouldn't need to update the tests at all, and that is a bad thing.

We want our tests to tell us when the API changes in important ways. For this reason, it is better to repeat yourself and say:

expect(result).to.equal({ food : 'sandwich' });

... which will yield the error expected undefined to equal {food : 'sandwich' }. It immediately tells you that I made a mistake somewhere in my code instead of silently breaking the behavior of the module.

[jniles]

Since these are constants, can we move them outside of this function? I feel like you structured your test similar to this code, but the code isn't well structured in this case.

[jniles]

Why is this download called 'VOUCHER_REGISTRY.TITLE'? I know if pre-existed your changes, but can you fix that?

[jniles]

An important part of tests is readability - and this isn't very readable to me. I propose that we clean this up by creating some extra variables. Like so:

const lotA = {
  uuid : 'd03e7870-0c8e-47d4-a7a8-a17a9924b123',
  lot : 'A',
  quantity : 50,
  expiration_date : '2019-01-01',
};

const lotB = {
  uuid : 'd03e7870-0c8e-47d4-a7a8-a17a9924b124',
  lot : 'B',
  quantity : 30,
  expiration_date : '2018-12-01',
};

Now that you have those, you can do this:

// a singe inventory with two lots
const mockStockFormStoreData = [{
  inventory_uuid : 'd03e7870-0c8e-47d4-a7a8-a17a9924b3f4',
  unit_cost : 0.5,
  lots : [ lotA, lotB ],
}];

const processed = Stock.processLotsFromStore(mockStockFormStoreData, entityUuid);
const [first] = processed;

const result = {
  uuid : 'd03e7870-0c8e-47d4-a7a8-a17a9924b123',
  lot : 'A',
  quantity : 50,
  expiration_date : '2019-01-01',
  inventory_uuid : 'd03e7870-0c8e-47d4-a7a8-a17a9924b3f4',
  unit_cost : 0.5,
  origin_uuid : entityUuid,
};

expect(first).to.deep.equal(result);

[jniles]

It will be helpful for future programmers to have a comment telling us what string this should produce. Something like:

// returns the string "/stock/lots/?type=csv&limit=100"
const query = Stock.getQueryString(FILTER_KEY_MOVEMENT, FILE_TYPE_CSV);

[jniles]

You can actually just use chai to do this - specifically the include() method. You would something like this work and save you from doing the for loop?

expect(query).to.include(`renderer=${FILE_TYPE_CSV}`);
expect(query).to.include('period=');
expect(query).to.include('limit=');

[jniles]

This is a good first attempt!

I propose you make a few lots/inventory items in the MockDataService (if they don't exist already) and use those in your tests. That way, this code can focus on the test itself, and spend less time setting up the tests.

[jniles]

Looks good to me! Nice and clean.

bors r+

[jniles]

Nice 👍

[bors]

Build succeeded

• continuous-integration/travis-ci/push