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

Add setup and teardown options to test function #128

Closed
wants to merge 1 commit into from

Conversation

hayd
Copy link
Contributor

@hayd hayd commented Jan 17, 2019

Stripped down from #108. This takes setup/teardown as optional arguments to test.

TODO:

  • agree on API
  • add tests

@bartlomieju
Copy link
Member

Having setUp/tearDown functions for single test is great, but I strongly advocate to add "global" setUp/tearDown that are run before any other test case (I think that's how it was implemented in #108).

@hayd
Copy link
Contributor Author

hayd commented Jan 18, 2019

You can share setup and teardown within a file, and perhaps i should have in the fileserver:

function fileServerTest(f: TestFunction) {
  test(t, { setup: startFileServer, teardown: killFileServer })
}

fileServerTest(function serveDirectory() { ... })
// etc

Similarly you could also also add a counter so that the setup/teardown only ran on the first/last test (defined inside a fileServerTest), BUT it's not quite as simple as you need to take account of the filtering... I don't see how to be more general here.

What you'd want is to define setup and teardown on the file which are run before/after all tests within that file (rather than within all tests), #108 did setup bit prior to every test, not just the tests in that file. This would be possible with denoland/deno#1189.

IMO this API doesn't preclude us from adding global/file setup/teardown later.

@bartlomieju
Copy link
Member

@hayd that's exactly what I meant, sorry for wrong phrasing

@43081j
Copy link
Contributor

43081j commented Jan 19, 2019

The common pattern across most languages is to have a sibling function/method which does setup, and the same for teardown.

It would be a bit more readable I think and concise if we did have;

setup(async function() { ... });
teardown(async function() { ... });
test(...);
test(...);

Those being global (across all files). With the introduction of a suite function to group tests, too, we would be able to have scoped setup/teardown functions:

suite(() => {
  setup(...);
  teardown(...);
  test(...);
  test(...);
});

Is there any particular reason we didn't go with this common pattern?

or are you saying support both these concepts? (options object & normal functions)

@hayd
Copy link
Contributor Author

hayd commented Jan 24, 2019

Perhaps:

suite(options: TestOptions, ...t: (TestDefinition | TestFunction)[]): void

is a little easier and would suffice?
I can add that.

@43081j
Copy link
Contributor

43081j commented Jan 24, 2019

It would be useful to know why you didn't choose to do what most JS test libraries currently do (sequential functions rather than options objects):

You seem to be very attached to your alternative pattern so it would be great to understand the choice there, if you wouldn't mind.

@ry
Copy link
Member

ry commented Jan 24, 2019

This seems to be a rather verbose way of doing setup/teardown. It would be great if somehow we could scope those to a file.... I suppose @43081j's suggestion could accomplish that - is there a way to do that without a suite ?

The way this patch is setup now doesn't seem to add much over just manually calling setup/teardown in each test function. (The only benefit is that the teardown will be called even if the test function hits an exception.)

@hayd
Copy link
Contributor Author

hayd commented Jan 24, 2019

If we had denoland/deno#1189 we could scope them to a test file, I can't see how else you'd do it.

(If we were scoping to a file then sequential is straightforward. That said, I think setup/teardown is sometime useful per function.)

@43081j
Copy link
Contributor

43081j commented Jan 24, 2019

Also if you have trouble with the magic other libraries do to scope those suite-specific function calls, you could have a context instead:

suite((context) => {
  context.setup(...);
  context.teardown(...);
  context.test(...);
  context.test(...);
});

Better than an options object and similar to what everyone is used to, and easier to strongly type.

@piscisaureus
Copy link
Member

I'm -1 on this. Our test runner is deliberately not a framework.
If we wanted a framework we could just pick mocha or jasmine or something.

Frameworks introduce a learning curve and test frameworks in particular tend to introduce new ways of doing things that you can do just as easily within vanilla javascript.

If you need setup/teardown for an individual test, I don't see what the advantage is over try...finally.
If you need a shared setup/teardown routine within a test, I'd recommend putting this on top of your file:

function serverTest(fn) {
  test({ name: fn.name, fn: async () => {
    await startServer();
    try { await fn(); }
    finally { await stopServer(); }
  }})
}

In other words, you can write write your own mini-framework inside a specific test file as much as you like, but let's keep the harness minimal as it is.

The only defensible case I think one could argue for is that sometimes you
need a shared setup/teardown routine for a group of tests. But this doesn't
address that anyway.

@43081j
Copy link
Contributor

43081j commented Jan 24, 2019

The important one to me is indeed having a shared setup for a group of tests. you can argue that anything can "just be done in vanilla js", but for a pattern as common as a test suite setup/teardown it may be worth providing what everyone will otherwise be forced to write (boilerplate).

personally i rarely use global test setups but do rely heavily on scoped ones (e.g. for setting up mocks and what not).

@ry
Copy link
Member

ry commented Jan 25, 2019

It would be nice to have per-file setup/teardown ... but only if it can be done in a very minimal way.

Thanks @hayd for the discussion starter, but I'm closing without merge.

@ry ry closed this Jan 25, 2019
@hayd hayd deleted the setup-teardown branch May 24, 2019 19:51
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

Successfully merging this pull request may close these issues.

5 participants