Make spy/stub use consistent

[willclarktech]

Expected behavior

We should follow a consistent pattern for stub use. We should make use of the sinon sandbox for ease of restoration. One option is to have a global afterEach hook which calls sinonSandbox.restore(), but this would require refactoring tests which make multiple it block assertions with a shared before hook spy/stub setup.

Actual behavior

We use a variety of patterns, not all of which make sense even besides the consistency issue. Some examples of the various patterns being used follow.

Creating spies/stubs in a describe block, e.g.:

https://github.com/LiskHQ/lisk/blob/699104486288d6380704ea3d22f30470480e96fa/test/unit/modules/rounds.js#L34-L42

Creating spies/stubs in a before hook and restoring in an after hook, e.g.:

https://github.com/LiskHQ/lisk/blob/699104486288d6380704ea3d22f30470480e96fa/test/unit/modules/rounds.js#L121-L134

Restoring/resetting in a beforeEach hook, e.g.:

https://github.com/LiskHQ/lisk/blob/8e6eb462a2181ffd327fe7b58757727e47177d87/test/unit/api/ws/workers/peersUpdateRules.js#L312-L333

Creating spies/stubs in a beforeEach hook, e.g.:

https://github.com/LiskHQ/lisk/blob/8e6eb462a2181ffd327fe7b58757727e47177d87/test/unit/logic/delegate.js#L95-L114

Assigning to a variable and calling sinon methods via that variable, e.g.:

https://github.com/LiskHQ/lisk/blob/8e6eb462a2181ffd327fe7b58757727e47177d87/test/unit/helpers/git.js#L26-L30

Stubbing without assignment and calling sinon methods directly on the stubbed method, e.g.:

https://github.com/LiskHQ/lisk/blob/8e6eb462a2181ffd327fe7b58757727e47177d87/test/unit/api/ws/workers/peersUpdateRules.js#L43

Reassigning a stubbed method to itself (redundant assignment), e.g.:

https://github.com/LiskHQ/lisk/blob/8e6eb462a2181ffd327fe7b58757727e47177d87/test/unit/api/ws/workers/peersUpdateRules.js#L147

Apparently unused spies/stubs/mocks, e.g.:

https://github.com/LiskHQ/lisk/blob/8e6eb462a2181ffd327fe7b58757727e47177d87/test/unit/logic/delegate.js#L91-L93

Which version(s) does this affect? (Environment, OS, etc...)

1.0.0

[MaciejBaj]

I prefer to have stubs initialization in before hooks of every tested function or global one for the test if it is possible to reuse it. It also contains default arguments for functions to be called with.

Then every following before hook of nested describe blocks can modify them to mirror a specific scenario that is being tested. It also modifies default arguments as needed.

Then in beforeEach hook the tested function is being called. If the function is:

• synchronous, just result,
• asynchronous, the result and error
  are/is being stored as a common for all of the tests of current scope parameter.

after hooks are cleaning what was malformed from the default state in the corresponding before hooks.

Advantages:
Expectations are usually 1 line long and reflect what's tested in test description. The code inside before hooks mirrors scenarios recreating in describe blocks.

Example:

    describe('remove', function () {

        var validPeer;
        var removeResult;
        var validLogicRemoveResult;

        before(function () {
            validLogicRemoveResult = true;
            validPeer = generateRandomActivePeer();
        });

        beforeEach(function () {
            peersLogicMock.remove = sinonSandbox.stub().returns(validLogicRemoveResult);
            removeResult = peers.remove(validPeer);
        });

        describe('when removable peer is frozen', function () {

            var originalFrozenPeersList;
            var loggerDebugSpy;

            before(function () {
                originalFrozenPeersList = _.assign({}, modulesLoader.scope.config.peers.list);
                modulesLoader.scope.config.peers.list = [{
                    ip: validPeer.ip,
                    wsPort: validPeer.wsPort
                }];
                loggerDebugSpy = sinonSandbox.spy(modulesLoader.scope.logger, 'debug');
            });

            after(function () {
                modulesLoader.scope.config.peers.list = originalFrozenPeersList;
                loggerDebugSpy.restore();
            });

            it('should not call logic.peers.remove', function () {
                expect(peersLogicMock.remove.called).to.be.false;
            });

            it('should call logger.debug with message = "Cannot remove frozen peer"', function () {
                expect(loggerDebugSpy.calledWith('Cannot remove frozen peer')).to.be.true;
            });
        });

        describe('when removable peer is not frozen', function () {

            it('should call logic.peers.remove', function () {
                expect(peersLogicMock.remove.calledOnce).to.be.true;
            });
...
        });
    });

[ManuGowda]

After a STM with @nazarhussain @willclarktech @4miners we conculded that we are not going to fix this issue and move the issue to backlog. For the short term any PR which has test using stub and spies need to complie with the following standard discussed in the meeting.

In each of the unit test, the test setup will have beforeEach hook with stubs and spies initialized and each it test case will have expect and after each test cases there will be afterEach hook which will restore the sandbox.

Here is an example from lisk-elements which demostrate the usage of stub

TestFile.js

describe('API method module', () => {

  beforeEach(() => {
    let requestResult = { success: true, sendRequest: true };
    resource = {
      request: sandbox.stub().resolves(requestResult),
    };
  });
  
  describe('#apiMethod', () => {
    describe('when no parameters are passed', () => {
      it('should request GET with default URL', () => {
        return handler().then(() => {
          expect(resource.request).to.be.calledOnce;
          return expect(resource.request).to.be.calledWithExactly(
            {
              method: GET,
              url: defaultFullPath,
              headers: defaultHeaders,
            },
            false,
          );
        });
      });
    });

    describe('when initialized with POST / parameters', () => {
      it('should call request with the given data', () => {
        return handler(firstURLParam, secondURLParam, { needed: true }).then(
          () => {
            expect(resource.request).to.be.calledOnce;
            return expect(resource.request).to.be.calledWithExactly(
              {
                method: POST,
                url: `${defaultFullPath}/${firstURLParam}/ids/${secondURLParam}`,
                headers: defaultHeaders,
                data: {
                  needed: true,
                  sort: 'id',
                },
              },
              true,
            );
          },
        );
      });
    });

    describe('when initialized with GET / parameters', () => {
      it('should be request with the given data', () => {
        return handler(firstURLParam, secondURLParam, { needed: true }).then(
          () => {
            expect(resource.request).to.be.calledOnce;
            return expect(resource.request).to.be.calledWithExactly(
              {
                method: GET,
                url: `${defaultFullPath}/${firstURLParam}/ids/${secondURLParam}?sort=id&needed=true`,
                headers: defaultHeaders,
              },
              false,
            );
          },
        );
      });
    });
  });
});

_global_hooks.js

afterEach(() => {
	return sandbox.restore();
});

@diego-G as discussed you can remove the milestone for this issue and keep it in backlog, once modularization is implemented and the tests are fixed as described by @willclarktech we can close the issue.

[yatki]

As we are migrating from mocha to jest, and also we recreated the application structure, we are already on the direction of making tests atomic. So it is ok to close this issue.