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

discussion: adopt baretest-like testing API #4092

Closed
bartlomieju opened this issue Feb 23, 2020 · 22 comments
Closed

discussion: adopt baretest-like testing API #4092

bartlomieju opened this issue Feb 23, 2020 · 22 comments

Comments

@bartlomieju
Copy link
Member

bartlomieju commented Feb 23, 2020

baretest is pretty interesting project that implements very simple testing framework (I wouldn't even call it a framework). It has a few functionalities which seem to be very desirable:

  • beforeAll/afterAll hooks - can be used to setup expensive stuff before testing (DB connections, net connections, etc.)
  • before/after hooks - can be used to implement useful sanity check, eg: verify that test closes all resources it opens; verify there are no pending ops when test finishes

Full code would look something like:

type TestHelper = () => void | Promise<void>;

export class TestSuite {
  _tests: TestDefinition[] = [];
  _only: TestDefinition[] = [];
  _skip: TestDefinition[] = [];
  _before: TestHelper[] = [];
  _after: TestHelper[] = [];
  _beforeAll: TestHelper[] = [];
  _afterAll: TestHelper[] = [];
  
  constructor(public name: string) {}
  
  test(name: string, fn: TestFunction): void {
    this._tests.push({ name, fn });
  }

  only(name: string, fn: TestFunction): void {
    this._only.push({ name, fn });
  }

  skip(name: string, fn: TestFunction): void {
    this._skip.push({ name, fn });
  }

  before(fn: TestHelper): void {
    this._before.push(fn);
  }

  after(fn: TestHelper): void {
    this._after.push(fn);
  }

  beforeAll(fn: TestHelper): void {
    this._beforeAll.push(fn);
  }

  afterAll(fn: TestHelper): void {
    this._afterAll.push(fn);
  }
}

And API:

const s = new Deno.TestSuite("my test suite");

let resourceSnapshot;
s.before(function () {
   resourceSnapshot = Deno.resources();
});

s.after(function () {
   const resources = Deno.resources();
   // diff with `resourceSnapshot` and ensure test
   // doesn't leak resources
   assertEquals(resourceSnapshot, resources, "test leaks resources");
});

s.test(function foobar() {
  ...
});

// TODO: fixme, temporary disabled
s.skip(function fizzbuzz() {
  ...
});

// Can also be run directly by `deno test` subcommand
await s.run();

Ref #4090

@bartlomieju
Copy link
Member Author

Just to clarify - Deno.test() would still exist.

This issue is about addition of new Deno.TestSuite() which makes built-in testing useful for both small or larger projects; while still keeping implementation dead simple.

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Feb 23, 2020

It would be nice if we could add these hooks while still using Deno.test(), closer to the way we do it now. It would take some strategy to be able to specify the scopes of the before()s and after()s, though.

Here's an idea:

let resourceSnapshot;
const s = new Deno.TestScope("my test suite", {
  beforeEach() {
    resourceSnapshot = Deno.resources();
  },
  afterEach() {
    const resources = Deno.resources();
    // diff with `resourceSnapshot` and ensure test
    // doesn't leak resources
    assertEquals(resourceSnapshot, resources, "test leaks resources");
  }
});

s.start();

Deno.test({
  name: "foobar",
  fn() {
    // ...
  }
});

// TODO: fixme, temporary disabled
Deno.test({
  skip: true,
  name: "fizzbuzz",
  fn() {
    // ...
  }
});

s.end();

@ecyrbe
Copy link
Contributor

ecyrbe commented Feb 23, 2020

Maybe i'm confused, but what about making Jest deno compliant ? is it too much work ?

@bartlomieju
Copy link
Member Author

@nayeemrmn s.start() and s.end() seem very funky are error-prone. I believe TestSuite is pretty common term in testing.

@ecyrbe Definitely; Jest has >50k lines of code and has >70 deps which are mostly Node specific. Besides it's full-blown testing framework with snapshots, mocking, etc.

@kevinkassimo
Copy link
Contributor

Bringing these hooks to Deno itself is fine I believe since they are truly minimal.

@nayeemrmn I think your proposal could be confusing, since there is no clear association between Deno.test() with the TestSuite instance s, and using s.start()/end() marker does not provide any enforcements pre-runtime. (and it is just so easy to forget to call s.end())

@ecyrbe Jest is just too huge a framework to port. To be honest I would rather put such effort to std/node polyfills than for Jest itself :) (maybe with enough Node polyfills we can get it work)

@nayeemrmn
Copy link
Collaborator

@bartlomieju How about giving it as an option to Deno.test():

let resourceSnapshot;
const suite = new Deno.TestSuite("my test suite", {
  beforeEach() {
    resourceSnapshot = Deno.resources();
  },
  afterEach() {
    const resources = Deno.resources();
    // diff with `resourceSnapshot` and ensure test
    // doesn't leak resources
    assertEquals(resourceSnapshot, resources, "test leaks resources");
  }
});

Deno.test({
  name: "foobar",
  suite,
  fn() {
    // ...
  }
});

// TODO: fixme, temporary disabled
Deno.test({
  skip: true,
  suite,
  name: "fizzbuzz",
  fn() {
    // ...
  }
});

Or have Deno.test() return some kind of symbol and giving those as a list to new TestSuite():

let resourceSnapshot;
const suite = new Deno.TestSuite("my test suite", {
  beforeEach() {
    resourceSnapshot = Deno.resources();
  },
  afterEach() {
    const resources = Deno.resources();
    // diff with `resourceSnapshot` and ensure test
    // doesn't leak resources
    assertEquals(resourceSnapshot, resources, "test leaks resources");
  },
  tests: [
    Deno.test({
      name: "foobar",
      fn() {
        // ...
      }
    }),

    // TODO: fixme, temporary disabled
    Deno.test({
      skip: true,
      suite,
      name: "fizzbuzz",
      fn() {
        // ...
      }
    })
  ]
});

Too many indents, probably. These are just examples of strategies to make Deno.test() the only way of declaring a test. I think that's important.

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Feb 23, 2020

@nayeemrmn IMHO I don't think adding a separate Deno.TestSuite can be that much of a cost. If we were to really only forced to maintain a single set of API, I would rather have Deno.TestSuite.default.test(), maintaining a static instance of TestSuite instead of keeping Deno.test.

Actually, I am actually thinking about making Deno.test() an alias to Deno.TestSuite.[defaultInstance].test(), and have things like Deno.test.beforeAll()/.afterAll()/.... In that sense, the tests declared through Deno.test() are implicitly on a default instance of TestSuite.

@ecyrbe
Copy link
Contributor

ecyrbe commented Feb 23, 2020

@bartlomieju Another silly question : so what about mocha + jspm as mocha does not rely on node and can run in the browser, and deno is almost like a browser : https://mochajs.org/#running-mocha-in-the-browser

@bartlomieju
Copy link
Member Author

@ecyrbe you can already run mocha with Deno; example in typeorm port for Deno: https://github.com/denolib/typeorm/blob/master/test/deps/mocha.ts

@ecyrbe
Copy link
Contributor

ecyrbe commented Feb 23, 2020

@bartlomieju So we already have access to mocha hooks ? why not use them ?

@kevinkassimo
Copy link
Contributor

@bartlomieju Actually I just noticed that deno is panicking if you try to import https://unpkg.com/mocha/mocha.js due to redirect to an absolute URL: https://unpkg.com/mocha/mocha.js redirects to /[email protected]/mocha.js, which cannot be parsed correctly. I'll see if I can do a quick fix for this.

@kevinkassimo
Copy link
Contributor

@ecyrbe Deno core should offer a set of bare minimal utilities, instead of being too opinionated IMO

@ecyrbe
Copy link
Contributor

ecyrbe commented Feb 23, 2020

@bartlomieju Thanks, that clarifies everything.

So i guess what deno expose today is sufficiently deadly simple and maybe we should orient users to use more complex libraries when needing advanced testing functionnality.

What i dislike here is adding another way to write deno native tests. If hooks are really necessary, i would then advise going all in and remove Deno.test() api.

@lucacasonato
Copy link
Member

What i dislike here is adding another way to write deno native tests. If hooks are really necessary, i would then advise going all in and remove Deno.test() api.

I agree - we should either provide only the dead simple Deno.test, or Deno.TestSuite, but not both as they don't work together - that would create confusion. I would personally vote for some sort of Deno.TestSuite, but I don't think a default instance is a good idea, because it might be confusing for new users that all Deno.test (or Deno.TestSuite.default.test() <- very long btw) are shared between modules. Also would you have to manually run() the default instance?

@kitsonk
Copy link
Contributor

kitsonk commented Feb 24, 2020

Personally, I don't feel we need to introduce a class to make all this happen.

How are capabilities like test setup and teardown work in Rust? That model seems to be very effective for us. I believe there is effectively block of code that are only included in the final binary and run when you do a test build, and the implementation of anything fancy is left up to the implementor.

I think we should really think minimalistic, otherwise we will be in the test harness business, not the JavaScript/TypeScript runtime business.

@ry
Copy link
Member

ry commented Feb 24, 2020

I agree with @kitsonk that we don't need setup/teardown. Rust's unit tests are a great example of this - they provide no such facility - and yet we're able to build very complex software on top of it.

We will not be introducing a more complex test facility for Deno. It should be simply registering functions to be run and checking if they get exceptions.

@keroxp
Copy link
Contributor

keroxp commented Feb 26, 2020

I agree that keeping it minimal, but as beforeAll and afterAll, we actually need them for std tests. Some integration tests certainly needs them for managing external processes / server. Currently there are no way to cleanup tests after test run. IMO Rust's test system is designed only for functional unit test.

I think Go is resolving the problem well by providing run hook.

func setup() {
    println("setup")
}

func teardown() {
    println("teardown")
}

func TestMain(m *testing.M) {
    setup()
    ret := m.Run()    
    teardown()
    os.Exit(ret)
}

@hayd
Copy link
Contributor

hayd commented Mar 1, 2020

You can write your own. I have used something like:

export async function test(t: Deno.TestDefinition) {
  async function wrapped() {
    await setUp();
    try {
      await t.fn();
    } finally {
      await tearDown();
    }
  }
  Deno.test({ name: t.name, fn: wrapped });
}

Perhaps something to aid that could be in std, but it's not necessary in Deno namespace.

@oscarotero
Copy link
Contributor

I have reached to this issue searching for support for beforeEach and afterEach. I understand the reason why you don't want to implement a new Deno.TestSuite() (or similar API). But maybe a more simple way to implement this is by extending the current Deno.test() allowing an array of tests and adding the setup and teardown functions to the TestDefinition. For example:

Deno.test({
  name: "example test",
  fn: [
    function test1() {
      assertEquals("hello", "hello");
    },
    function test2() {
      assertEquals("world", "world");
    }
  ],
  beforeEach() {
    // Code to execute before each test function
  },
  afterEach() {
    // Code to execute after each test function
  },
  beforeAll() {
    // Code to execute before all test functions are executed
  },
  afterAll() {
    // Code to execute after all test functions are executed
  }
});

@timreichen
Copy link
Contributor

How about adding nested Deno.test() and await Deno.test() support?

Deno.test("suite", () => {
  // beforeAll
  const tests = [
    {
      name: "test1",
      fn: () => {
        
      }
    },
    {
      name: "test2",
      fn: () => {
        
      }
    }
  ]
  
  for await (const test of tests) {
    // beforeEach
    await Deno.test(test)
    // afterEach
  }
  // afterAll
})

console:

test suite
  test test1 ... ok
  test test2 ... ok

That way we would be able to implement the upper described functionality without adding any new syntax.

@nayeemrmn
Copy link
Collaborator

@timreichen I think that's a really nice solution :-) Can you open a new issue? Use the code example more like the one here: https://discord.com/channels/684898665143206084/684911491035430919/762300901590433812.

@KyleJune
Copy link
Contributor

KyleJune commented Oct 4, 2020

Here is a wrapper I implemented for adding test setup/teardown to Deno.test. I based it on the idea of having a TestSuite from this discussion. I didn't make an issue suggesting it because it seemed to already be decided that deno doesn't need setup/teardown built in.

https://deno.land/x/test_suite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests