Vscode-extension-for-zowe: Unit Test Refactoring: Possible standards for writing tests

[katelynienaber]

Here are proposed standards required in all new unit tests:

• Each _describe_ block should test only one function
• Each _it_ block should test only one test case (contain only one call to the tested function)
• The _describe_ block description should contain the name of the function being tested, and the class it belongs to (ie. "Tests for function USSNode.getChildren")
• Each _describe_ block should contain the following blocks:

jest.afterEach(() => { jest.clearAllMocks(); });
jest.afterAll(() => { jest.restoreAllMocks(); });

What do you guys think about these rules? Should their enforcement be automated? Changes? Thoughts?

[ghost]

Regarding the proposal to require these blocks:

jest.afterEach(() => { jest.clearAllMocks(); });
jest.afterAll(() => { jest.restoreAllMocks(); });

Additional changes must be made to our unit tests for the above code blocks to be effective. According to Jest docs:

Beware that jest.restoreAllMocks() only works when the mock was created with jest.spyOn; other mocks will require you to manually restore them.

Currently many of our mocks require manual restoring, because we manually create the mocks by overriding functions with jest.fn().

When our mocks are not restored, we experience problems like #710. (Tests fail to run with the --runInBand option, because test reporters like jest-stare are running in the same Node process as the unit tests, and trying to use the fs module which still has active mocks.)

If we want to make use of jest.restoreAllMocks(), we should consider creating our mocks with jest.spyOn instead of our current approach.

[jellypuno]

I think @zFernand0 did great in re-factoring the Profiles Unit Testing. It is readable and understandable. After adding the Edit Profiles feature, it was easy for me to add new tests. 👍

[Colin-Stone]

I can't find anything inherently wrong with the proposals and discussion points. I'm concerned if we are over-focussing on testing currently.
Within an Agile framework we should be seeing code going out regulalrly based upon a perceived opportunity or specific customer requirement. I believe we need adequate testing and what I mean by adequate is it protects you if someone does something that breaks or impacts your code.
The word "enforcement" makes me shiver. (but that's my problem) I do want to follow good guidelines and techniques, that's how I do my coding so please create the examples and samples and make it easy for us to follow your lead.
My motivation is to get people to participate so we get their great ideas and build stuff customers want. I'm just a little bit concerned we might put them off if we seem too overly focussed on testing.

[lauren-li]

I agree with the first 3 bullet points in this issue. Regarding the last bullet point, I will defer to how the team decides is the best way to do this, but generally speaking, I agree that we should ensure unit tests don't bleed into one another with their processing/results. I second @Colin-Stone on his point that having examples/samples for how to follow the decided-upon best practices would be extremely helpful for encouraging the contribution of quality code.

[katelynienaber]

@tjohnsonBCM I will use jest.spyOn on all mocks in the current PRs and add some notes to this post once I try it...

[katelynienaber]

@Colin-Stone @lauren-li I will write up a template for making test files, that we can share on the wiki and maybe here on GitHub.

Ofc edits can be added as needed.

[VitGottwald]

I think tests should be self contained and as easy to read as possible. So my preference would be to have as few beforeEach/afterEach and clearAllMocks/restoreAllMocks as possible.

[crawr]

I agree with the first three bullet points as they are pretty straightforward.

For the last point, I agree with above comments that a template and/or concrete examples will go a long way towards better understanding and compliance. As a contributor, I tend to create tests that have similar structure and style to what is already existing in the module. As a reviewer, it will be easier to read if the tests at least in the same file are consistent.

Additionally, I want to highlight that the motivation behind this Unit Test Refactoring effort is to increase maintainability of the tests themselves and to improve our velocity in developing code.
Personally, in the past, I spent a significant amount of time after I developed some functionality to maintain previous tests and to define new objects for my code. So if these standards and subsequent implementation will alleviate that, I think it is worth focusing on these unit tests today so that contributors will find it easier and faster to add new and exciting features going forward.

[katelynienaber]

Some new changes that have been on the drawing board between @stepanzharychevbroadcom, @VitGottwald and myself:

• describe() block description should contain class name & name of function being tested
  _(Same as before)_
• Each describe() block should have a beforeEach() block as in the provided template
  _(Some changes were made to the template)_
• Each it() block should contain only one call to the function being tested
  _(Same as before)_
• Core node.js modules must be mocked explicitly, according to the docs
  _(This should fix the jest-stare issue we were having in future)_
• Only break these rules if you have no other option!

[katelynienaber]

The following contains the official text we would like to put on the Wiki, as well as 2 templates for writing tests:
Writing Unit Tests.zip

[katelynienaber]

Basically, the template looks as below. We removed the afterEach/afterAll blocks and are simply redeclaring the whole environment before each it() block.

This means that:

• We would no longer depend on jest's clearing functions, so hopefully we won't get weird side-effects from incorrect usage of clearAllMocks() or resetAllMocks()
• Reusable variables will always be reset to their starting values, so @crawr can use a global variable and be sure it hasn't been tampered with before it reaches her test

import * as vscode from "vscode";
// More imports here

// Node standard modules must be mocked explicitly:
jest.mock('fs');

async function generateEnvironmentalMocks() {
    const environmentalMocks = {
        someFunction: jest.fn(),
        someVariable: "someValue"
        // ...more global variables here...
    };

    // Set properties for global mocks here

    Object.defineProperty(vscode.workspace, "onDidSaveTextDocument", { value: "someValue", configurable: true });

    return environmentalMocks;
}

describe("<<class name here>> Unit Tests - Function <<function name here>>", () => {
    let environmentalMocks;
    let blockMocks;

    beforeEach(async () => {
        environmentalMocks = await generateEnvironmentalMocks();
        blockMocks = await generateBlockMocks();
    });

    async function generateBlockMocks() {
        const newMocks = {
            foo: "bar",
            // ...more block variables here...
        };

        // Set properties for block mocks here

        return newMocks;
    }

    it("Tests that something occurs...", async () => {
        // Use blockMocks.foo or environmentalMocks.propertyName here in your tests
    });
});

[VitGottwald]

@katelynienaber this is really nice, especially the absence of clearAllMocks/restoreAllMocks. And I think it can be improved even a bit more, by moving the generate* function calls at the beginning of every individual test. Then we could get rid of the shared mutable variables environmentalMocks and blockMocks on the _describe block level_. That will give us truly independent tests that cannot easily interfere with each other.

[katelynienaber]

@katelynienaber this is really nice, especially the absence of clearAllMocks/restoreAllMocks. And I think it can be improved even a bit more, by moving the generate* function calls at the beginning of every individual test. Then we could get rid of the shared mutable variables environmentalMocks and blockMocks on the _describe block level_. That will give us truly independent tests that cannot easily interfere with each other.

Ya for sure I can move that as well...please check my PRs in a bit :)

[VitGottwald]

One more suggestion, I think using create* instead of generate* would be better because generators have a very particular meaning in javascript https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Generator

[katelynienaber]

Okay. So I am renaming it like:

• The folder for mocked variable generators is called mockCreators
• The function for making global mocks is called createGlobalMocks and can return object globalMocks
• The function for making block-specific mocks is called createBlockMocks and can return object blockMocks

[katelynienaber]

Can we close this issue??

[zdmullen]

This issue can also be closed. See the doc for further details on unit test guidance: https://github.com/zowe/vscode-extension-for-zowe/wiki/Best-Practices:-Unit-Test-Structure