hedera-sdk-js icon indicating copy to clipboard operation
hedera-sdk-js copied to clipboard

TCK for Wallets and Signers

Open janaakhterov opened this issue 2 years ago • 3 comments

While working on the TCK I obviously want to optimize for both user and developer experience. With the TCK this is more challenging to determine than usual. The summary here is that we have two options which both have pros and cons for maintainability and usability. I've already implemented option 1, but I feel option 1 will become really difficult to maintain once we start supporting more testing frameworks (and once more testing frameworks are created). However, option 1 does require less code to be added before using the TCK from the users perspective, so maybe we want to just bite the bullet and use option 1 for now. Another thing to mention is these options are not compatible nor integrate-able later. Meaning if we choose option 1 officially, we cannot later change this to option 2 without breaking changes.

Edit: Option 2 is now implemented, we can go back to option 1 at any time, but option 2 I think is much better now that I've let it brew in my mind.

Option 1: Create a TCK per testing framework

Pros:

  • Source code uses the testing framework directly
  • Requires less initialization code to use the TCK from the user's side Cons:
  • Duplicates the bulk of the testing between each testing framework The only code that really differs between testing frameworks is the expect clauses.

Option 2: Create a singular TCK which uses callbacks for assertions

Pros:

  • Makes source code for TCK testing framework independent
  • Removes duplicate code between testing frameworks Cons:
  • Making the TCK testing framework independent can also be considered a con here since you can argue it makes developing the TCK more difficult as it's less straight forward.
  • Requires much more initialization code as the user is required to essentially provide a map of e.g. expectToEqual => (a, b) => { expect(a).to.be.equal(b) } in chai + mocha or expectToEqual => (a, b) => { expect(a).toEqual(b) } in jest.

Note: Both options already require a little bit of code from the user to get the TCK working, for instance, I currently have implemented option 1, and this is what I had to add to get the TCK working

import * as tck from "@hashgraph/tck-chai";
import { SimpleRestSigner } from "../src/signer";
import mocha from "mocha";

describe("TCK", async function () {
    const self = this;
    self.timeout(60 * 1000);

    this.beforeAll(async function () {
        const signer = await SimpleRestSigner.connect();
        const tests = tck.createTckTests(signer, (request) => {
            // This is the callback that will be called after each request is created
            // It is up to the signer/provider implementor to decide what to do here.

            const provider = signer.getProvider()!;
            provider.instance.post("/wallet/callback", { request });
        });

        for (const test of tests) {
            self.addTest(new mocha.Test(test.name, test.fn));
        }
    });

    // A single test is required, otherwise `beforeAll` is never run and tests
    // are added
    it("can connect", async function () {});
});

If we were to choose option 2, then we'd need users to write something like:

import * as tck from "@hashgraph/tck-chai";
import { SimpleRestSigner } from "../src/signer";
import mocha from "mocha";

describe("TCK", async function () {
    const self = this;
    self.timeout(60 * 1000);

    this.beforeAll(async function () {
        const expects = {
            toEqual => (a, b) => { expect(a).to.deep.equal(b) },
            toHaveMembers => (a, b) => { expect(a).to.have.members(b) },
            toNotBeNull => (a) => { expect(a).to.not.be.null },
            toBeTrue => (a) => { expect(a).to.be.true },
        };
        const signer = await SimpleRestSigner.connect();
        const tests = tck.createTckTests(signer, (request) => {
            // This is the callback that will be called after each request is created
            // It is up to the signer/provider implementor to decide what to do here.

            const provider = signer.getProvider()!;
            provider.instance.post("/wallet/callback", { request });
        }, expects);

        for (const test of tests) {
            self.addTest(new mocha.Test(test.name, test.fn));
        }
    });

    // A single test is required, otherwise `beforeAll` is never run and tests
    // are added
    it("can connect", async function () {});
});

Also, this PR contains fix for #1123 since I needed to make sure Transaction.[from|to]Bytes() worked as expected before testing via TCK.

janaakhterov avatar Jun 13 '22 20:06 janaakhterov

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
0.5% 0.5% Duplication

sonarqubecloud[bot] avatar Jul 14 '22 22:07 sonarqubecloud[bot]

cc @rbair23 @SimiHunjan @gregscullard @sergmetelin I'd like your input here (check PR description)

janaakhterov avatar Sep 01 '22 21:09 janaakhterov

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 18 Code Smells

No Coverage information No Coverage information
0.3% 0.3% Duplication

sonarqubecloud[bot] avatar Sep 06 '22 20:09 sonarqubecloud[bot]