Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Continuous] Test Suite Improvements #2407

Open
faustbrian opened this Issue Apr 12, 2019 · 0 comments

Comments

Projects
None yet
1 participant
@faustbrian
Copy link
Collaborator

faustbrian commented Apr 12, 2019

Now that all tests are in one place and separated by type more improvements can gradually be worked on. This list will be updated, expanded and modified as work on test continues.


Reduce nesting & better phrasing of tests and the expected outcome

We have quite a few tests that look like the following which results in meaningless descriptions and unnecessary nesting.

describe("NetworkMonitor", () => {
    describe("discoverPeers", () => {
        it("should be ok", () => {});
        it("should not be ok", () => {});
    });

    ...
});

Instead of structuring tests like this we should make it more clear what a test is supposed to do and expect as outcome.

describe("NetworkMonitor", () => {
    it("should discovery 10 peers and suspend 3 because of a timeout", () => {});
    it("should discovery 10 peers, validate and accept all", () => {});

    test("discovers 10 peers and suspends 3 because of a timeout", () => {});
    test("discovers 10 peers, validates and accepts all", () => {});

    ...
});

A test for discoverPeers that reads it should be ok doesn't give much information but it should discovery 10 peers and suspend 3 because of a timeout or it should discovery 10 peers, validate and accept all are much more informative of what they do and expect to be the outcome.

Choose one style of naming and stick with it, either use the it method and formulate it with should do this ... and expect ... that or use test like in the above example to describe a scenario.

Gradually reduce the number of mocks

At the moment a lot of tests rely on heavy mocking which is a sign of code smell and partly caused by the late implementation of the container which resulted in a hacky implementation rather than everything being built around it from day 1.

Over time tests should rely less and less on vast amounts of mocking and instead only require mocking for isolated parts rather than having fully mocked plugins like the container.

Gradually reduce the number of unit tests

Unit tests are all fine and dandy to test behaviour and edge-cases based on specific input but don't guarantee that is the actual behaviour once they are integrated into the system.

A better approach is to have integration tests cover the same scenarios instead. You still get the same amount of code coverage and also have the confidence that your unit works as expected under different conditions once integrated into the full system.

There are dozens of articles on integration vs. unit testing and the RoI on each type that can be easily googled to look at the pros and cons of each.

More robust functional tests

Functional tests for their initial implementation rely on block time based sleeps which can be brittle if tests are ran on a slow machine. Instead they should rely on the event emitter or periodic checks to know if blocks have been forged before continuing with assertions instead of relying on block time based sleeps.

Migrate all tests to unitnet

Currently tests use a mix of testnet and unitnet which is the result of a late switch to unitnet for a few parts of the test suite to make things easier. Instead everything should use unitnet so testnet stays unaffected by changes required for tests.

This requires all fixtures and mocks to be regenerated for the unitnet configuration.

Better random data generation

At the moment there are randomly issues with delegate registration and vote tests because sometimes there are duplicate usernames or public keys which result in failures for example when forging related tasks are tested so we need to ensure that all data is unique to make them pass every time.

Remove unused imports

At the moment jest-extended is imported in basically every file even though it should be enough to be in setupFilesAfterEnv but TypeScript throws errors about not knowing certain matchers.

Miscellaneous

  • Create block fixtures for each network
  • Create transaction fixtures for each network
  • Create identity fixtures for each network
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.