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

test(common): increase test coverage #309

Open
petermetz opened this issue Oct 13, 2020 · 6 comments
Open

test(common): increase test coverage #309

petermetz opened this issue Oct 13, 2020 · 6 comments

Comments

@petermetz
Copy link
Contributor

@petermetz petermetz commented Oct 13, 2020

Description

As a community member of Cactus I want to feel good about using software that has good test coverage so that I can sleep a little bit better (not much because severe bugs can still lurk in codebases with 100% test coverage, but nevertheless...)

Currently if you run the test command for the common package (see below) then you get a coverage report is is mostly in the red. The idea for this issue is to turn that green.

Execute the tests of the common package

npx tap --jobs=1 --timeout=60 "packages/cactus-common/src/test/typescript/{unit,integration}/"

The report at the end of the test execution:

Suites:   5 passed, 5 of 5 completed
Asserts:  47 passed, of 47
Time:     10s
--------------------------|----------|-------------------|
File                      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
--------------------------|----------|-------------------|
All files                 |    82.51 |    67.61 |       50 |    82.35 |                   |
 main/typescript          |    86.01 |    72.88 |    52.94 |    86.15 |                   |
  bools.ts                |       50 |        0 |       50 |                13 |
  checks.ts               |       40 |        0 |       40 |          16,17,18 |
  coded-error.ts          |       20 |        0 |       25 |            7,8,12 |
  js-object-signer.ts     |    96.77 |    91.67 |      100 |    96.77 |                34 |
  key-converter.ts        |      100 |    96.15 |      100 |                83 |
  objects.ts              |     62.5 |    77.78 |       50 |    66.67 |    57,58,59,60,63 |
  public-api.ts           |      100 |    36.36 |      100 |                   |
  secp256k1-keys.ts       |      100 |                   |
  strings.ts              |    28.57 |        0 |    28.57 |     9,13,17,18,19 |
 main/typescript/logging  |     69.7 |    41.67 |    42.86 |     69.7 |                   |
  logger-provider.ts      |    71.43 |    42.86 |    33.33 |    71.43 |       26,27,28,29 |
  logger.ts               |    68.42 |       40 |    45.45 |    68.42 | 46,50,54,58,61,67 |
 test/typescript/fixtures |    71.43 |      100 |       50 |    71.43 |                   |
  dummy-classes.ts        |    71.43 |      100 |       50 |    71.43 |             11,24 |
--------------------------|----------|-------------------|

Acceptance Criteria

  1. The first line (All files) is all green meaning that it's above threshold (I think the limit for green is 85% but not a 100% sure on that)
  2. Tests added use the tape library (not tap)
  3. Tests added follow the existing conventions

cc: @jonathan-m-hamilton

@eLeontev
Copy link

@eLeontev eLeontev commented Oct 18, 2020

@petermetz what is the reason to use tape/tap instead of common used jest? Looks like tape does not support from the box a lot of required functionality (beforeEach/spies/flexible matchers, etc).
Are you ok to add tests for common on jest?

@gabrielsoaresm94
Copy link

@gabrielsoaresm94 gabrielsoaresm94 commented Oct 19, 2020

@petermetz I'm trying to do the task with the documentation I found on github and also based on the tests that have already been created, everything ok?

@petermetz
Copy link
Contributor Author

@petermetz petermetz commented Oct 19, 2020

@petermetz what is the reason to use tape/tap instead of common used jest? Looks like tape does not support from the box a lot of required functionality (beforeEach/spies/flexible matchers, etc).
Are you ok to add tests for common on jest?

Test Anything Protocol and in general to have tests and a testing framework that is simple and lightweight even if some of the cool features are missing. Does Jest support TAP?

P.S.: The reason we have both tap and tape is because we are in the middle of a slow, gradual migration from tap to tape because only tape supports running tests in browsers.

@petermetz
Copy link
Contributor Author

@petermetz petermetz commented Oct 19, 2020

@petermetz I'm trying to do the task with the documentation I found on github and also based on the tests that have already been created, everything ok?

* https://github.com/substack/tape

* https://ci.testling.com/guide/tape

@eLeontev are aligned and not working on the same thing in parallel. ;-)

@gabrielsoaresm94
Copy link

@gabrielsoaresm94 gabrielsoaresm94 commented Oct 29, 2020

I managed to create some unit tests, but I have a problem to test the Logger, since its methods return void. I was able to instantiate the class to call the method and see the log appear, but not pass the test yet. Any tips on how to use the tape to listen to a function?

@petermetz
Copy link
Contributor Author

@petermetz petermetz commented Oct 29, 2020

I managed to create some unit tests, but I have a problem to test the Logger, since its methods return void. I was able to instantiate the class to call the method and see the log appear, but not pass the test yet. Any tips on how to use the tape to listen to a function?

@gabrielsoaresm94 Awesome, thank you for all your efforts so far!
I've never done this myself either, but I can definitely recommend that you take a look at this specific part of the NodeJS documentation where they talk about process std out/err and console.log/error working:
https://nodejs.org/api/process.html#process_process_stdout

Based on what's described on the above link, this is just made up pseudo-ish code I can imagine being a workable solution blueprint. Let me know if this is good enough guidance!

logger.test.ts

/ import dependencies...
test("logger writes to stdout", async(t: Test) => {

const log = LoggerProvider.getOrCreate({ level: "TRACE", label: "logger-test" });

/ generate random UUID v4 to guarantee we don't mistake something else as the marker
const marker = uuidv4();

/ hook up to the stdout data stream and wrap it in a promise that can be awaited 
/ for when the marker does appear on stdout (which would be a passing test)
/ or when it times out (which would mean the test is failing).
/ Certain issues could happen here if the stream is chunking data and then you never
/ actually get the complete marker string at once but instead different parts of it
/ but I did not consider this because the uuid is only a few dozen bytes when stored as a hex string 
/ so I'm pretty confident it wouldn't get chunked (probably not impossible either though so
/ if you are paranoid about that happening (which would make the test flaky) then you can
/ bake in some stream data aggregation instead where you collect and continually append
/ the incoming data chunks and test for marker presence in the aggregate variable not the chunk
/ that is provided in the 'data' event handler callback.
  const promise = new Promise(resolve, reject) => {
     const timerId = setTimeout() => reject(new Error('Timed out waiting for marker to appear on stdout');
     process.stdout.on('data', (data: Buffer) => {
        if (data.toString('utf-8').includes(marker) {
        clearInterval(timerId);
         resolve(marker);
         }
    });
});

/ send the log now that we have hooked into the stream waiting for the marker to appear
log.info(marker);

/ wait for the marker to appear on stdout OR crash with timeout if it never comes
await promise; 
});

/ rinse and repeat for stderr
test("logger writes to stderr", ...)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.

Follow Lee on X/Twitter - Father, Husband, Serial builder creating AI, crypto, games & web tools. We are friends :) AI Will Come To Life!

Check out: eBank.nz (Art Generator) | Netwrck.com (AI Tools) | Text-Generator.io (AI API) | BitBank.nz (Crypto AI) | ReadingTime (Kids Reading) | RewordGame | BigMultiplayerChess | WebFiddle | How.nz | Helix AI Assistant