unional / ava-fixture

Fixture test for https://github.com/avajs/ava
MIT License
5 stars 1 forks source link

beforeEach (and maybe others) don’t get passed the case path #9

Closed flying-sheep closed 7 years ago

flying-sheep commented 7 years ago
const ftest = fixture(ava, 'some/path')

ftest.beforeEach(async (t, p) => {
  console.log(p)
})

ftest('casename', (t, p) => {})
ftest('casename2', (t, p) => {})

always logs undefined. I’d expect it to log some/path/casename and some/path/casename2

my specific use case is of course:

ftest.beforeEach(async (t, p) => {
  t.context.thing = await doSomethingWith(p)
})

ftest('casename', t => {
    t.truthy(t.context.thing, ...)
})
flying-sheep commented 7 years ago

i’d expect the API to be:

fixture(ava: Ava.Test, absOrRelativePath: string): FixtureTest
// the casePath argument is a string
fixtureTest([title], caseName, (t: Ava.Test, casePath: string) => Promise<any> | void): void

// test modifiers of course don’t change the test signature
fixtureTest.serial(...): void
fixtureTest.cb(...): void
fixtureTest.only(...: void
fixtureTest.skip(...): void
fixtureTest.failing(...): void

fixtureTest.only.failing.serial(...): void

// test placeholder
fixtureTest.todo(title: string): void

// those setup functions are run once before/after all tests and therefore can’t receive fixture paths.
fixtureTest.before([title], (t: Ava.Test) => Promise<any> | void): void
fixtureTest.after([title], (t: Ava.Test) => Promise<any> | void): void

// setup functions run per test should receive the tests’ fixture paths.
fixtureTest.beforeEach([title], (t: Ava.Test, casePath: string) => Promise<any> | void): void
fixtureTest.afterEach([title], (t: Ava.Test, casePath: string) => Promise<any> | void): void

also chaining modifiers should work

unional commented 7 years ago

I wrote this is TypeScript directly, and making chaining work seems to be quite hard. Do you have an suggestion?

As for the beforeEach(), I didn't add d into the call back because I didn't see the need of it.

Also, the API in the readme is outdated. See the actual code completion for more info. I'll be updating the readme.

In short, it now does (if you do baseline test):

import test from 'ava'
import fixture from 'ava-fixture'

const ftest = fixture(test, 'fixture', 
  { casesPath: 'cases', baselinesPath: 'baselines', resultsPath: 'results' })

ftest('test', 'case-1', (t, d) => {
  t.is(1, 1)
  console.log(d.casePath) // resolved case path for 'case-1'
  console.log(d.baselinePath)
  console.log(d.resultPath)
  return d.match() // will match baseline and result folder
})
flying-sheep commented 7 years ago

well, FixtureTest has to be more than a plain function.

we need something implementing something like this signature:

// Ava.ContextualTest = (t: Ava.ContextualTestContext) => PromiseLike<void> | Iterator<any> | Observable | void
// Ava.ContextualCallbackTest = (t: Ava.ContextualCallbackTestContext) => void
type FixtureTest = (t: Ava.ContextualTestContext, casePath: string) => PromiseLike<void> | Iterator<any> | Observable | void
type CallbackFixtureTest = (t: Ava.ContextualCallbackTestContext, casePath: string) => void

interface FixtureSetupOnce {
    (run: Ava.ContextualTest): void
    (title: string, run: Ava.ContextualTest): void

    cb: CallbackFixtureSetupOnce
    serial: FixtureSetupOnce
    only: FixtureSetupOnce
    skip: FixtureSetupOnce
}

interface CallbackFixtureSetupOnce {
    (run: Ava.ContextualCallbackTest): void
    (title: string, run: Ava.ContextualCallbackTest): void

    serial: CallbackFixtureSetupOnce
    only: CallbackFixtureSetupOnce
    skip: CallbackFixtureSetupOnce
}

interface FixtureSetupEach {
    (run: FixtureTest): void
    (title: string, run: FixtureTest): void

    cb: CallbackFixtureSetupEach
    serial: FixtureSetupEach
    only: FixtureSetupEach
    skip: FixtureSetupEach
}

interface CallbackFixtureSetupEach {
    (run: CallbackFixtureTest): void
    (title: string, run: CallbackFixtureTest): void

    serial: CallbackFixtureSetupEach
    only: CallbackFixtureSetupEach
    skip: CallbackFixtureSetupEach
}

interface FixtureTest {
    (caseName: string, run: FixtureTest): void
    (title: string, caseName: string, run: FixtureTest): void

    cb: CallbackFixtureTest
    serial: FixtureTest
    only: FixtureTest
    skip: FixtureTest

    failing: FixtureTest
    todo: (title: string) => void

    before: FixtureSetupOnce
    after: FixtureSetupOnce
    beforeEach: FixtureSetupEach
    afterEach: FixtureSetupEach
}

interface CallbackFixtureTest {
    (caseName: string, run: CallbackFixtureTest): void
    (title: string, caseName: string, run: CallbackFixtureTest): void

    serial: CallbackFixtureTest
    only: CallbackFixtureTest
    skip: CallbackFixtureTest

    failing: CallbackFixtureTest

    before: CallbackFixtureSetupOnce
    after: CallbackFixtureSetupOnce
    beforeEach: CallbackFixtureSetupEach
    afterEach: CallbackFixtureSetupEach
}

see also

and ava uses code generation to create the full signature from this: https://github.com/avajs/ava/blob/master/types/make.js

unional commented 7 years ago

Yes. ava is written in JavaScript and generating the typings using a script. ava-fixture is written in TypeScript directly so that is different.

Anyway, I have cleaned up the API. callback, before, after, beforeEach, afterEach, serial, and only are removed. For before, after, beforeEach, afterEach, only use ava directly. For callback, serial, I don't see the need of it. Feel free to discuss and if you have a need for it, then we can add them back.

unional commented 7 years ago

I just released 0.6.

Chain between only(), skip(), failing() actually works, but I need to figure out how to specify the interface, maybe something like you proposed.

But the code is working:

ftest.only.failing()
ftest.skip.failing()
ftest.failing.only()
ftest.failing.skip()
unional commented 7 years ago

Released 0.7. It should be ok now. Feel free to continue the discussion.

flying-sheep commented 7 years ago

will process.cwd() already point to the casePath in ava.before()?

unional commented 7 years ago

Nope. It only does that in the callback of the actual test.

It's not likely you would do anything file related in the before() as it is per file, not per test case.

It may make sense to do it in beforeEach() and afterEach(). Is there a case for it?

flying-sheep commented 7 years ago

sorry, typo: of course I meant beforeEach. the use case is in the first comment

unional commented 7 years ago

I thought about it but can't find a way to implement.

The problem is the actual case is specified in the test, so the fixture doesn't know which test will be run thus can't get the path to the specific case. i.e.:

import test from 'ava'
import fixture from 'ava-fixture'

const ftest = fixture(test, 'fixture/cases')

ftest.beforeEach(t => {
  // Don't know which test will be run next.
})

// 'case-1' is specified here.
ftest('case-1', (t, d) => {
})
unional commented 7 years ago

Unless the api changes to this:

import test from 'ava'
import fixture from 'ava-fixture'

const ftest = fixture(test, 'fixture/cases', 'fixture/baselines', 'fixture/results', 
['case-1', 'case-2', ...])

ftest.beforeEach(...)

ftest.next((t, d) => { ... })

Which is horrible and defeat the purpose of ava as now they have to run in serial.

unional commented 7 years ago

If you are looking for

ftest.beforeEach((t, d) => {
  d.casesPath // abs path of cases parent folder
  d.baselinesPath // abs path of baselines parent folder
  d.resultsPath // abs path of results parent folder
})

then it will be possible. Would that be helpful?

The beforeEach() should not know anything about the actual case anyway.

flying-sheep commented 7 years ago

true, maybe it’s not possible while reusing ava’s beforeEach/afterEach.

if we wanted it (and as you said we might not as beforeEach/afterEach maybe shouldn’t know about test cases) we’d have to reimplement beforeEach/afterEach somewhat like this:

return testfn(`${title ? title + ' ' : ''}(fixture: ${caseName})`, (t: ContextualTestContext) => {
    …
    try {
        …
        for (const beforeEach of beforeEachRegistry) beforeEach(t, d)
        result = run(t, d)
        …
        for (const afterEach of afterEachRegistry) afterEach(t, d)
    } finally {
        for (const afterEachAlways of afterEachAlwaysRegistry) try {
            afterEachAlways(t, d)
        } catch (e) { console.error(e) }
        process.chdir(cwd)
    }
    …
})
unional commented 7 years ago

I see, have to register all calls first before invoking the tests... Seems like quite a lot of work. 😏

unional commented 7 years ago

I think no it could get very hairy.

Consider Ava run things in parallel and we have to use ava beforeEach() and afterEach() to get notify each test is completed so we can run our versions.

flying-sheep commented 7 years ago

OK, let’s get fundamental! 😄

for me, fixtures are about DRYness and decoupling code and data: they should be used if your test code is easy to reason about, and a bunch of your test cases are best represented as homogeneous files or directories (i.e. node packages your package operates on, or source files in some format that your package transforms)

this means that i really want to do sth. like.:

test(fixturedir, async t => {
    const fixtures = await glob(`${fixturedir}/*`)
    for (const fixture of fixtures) {
        t.test(fixture, async t => {
            const names = ['source', 'target'].map(f => path.join(fixture, f))
            const [source, target] = await Promise.all(names.map(fs.readFile))
            t.is(source, target)
        })
    }
})

this library makes it easier to manually add one test per fixture, but doesn’t support the above.

maybe i should write another one

unional commented 7 years ago

Seems like what's missing to support your use case is the outer test() you described, which is just a simple function. The t.test() can already handled by current code.

unional commented 7 years ago
function wrap(path, cb) {
  process.chdir(path)
  cb()
}

something like that.

unional commented 7 years ago

Seems like your use case is like this?


fixture(ava, paths..., async (t, d) => {
  process.cwd() // casePath
  // do things
   await do(from d.casePath to d.resultPath)

  return d.match() // d.resultPath, d.baselinePath
})
unional commented 7 years ago

If that's what you are looking for, then the only skip failing may not really apply because the test suite is going through all case folders for you. So can can't only or skip particular one of them.

flying-sheep commented 7 years ago

could you please shortly explain what case and baseline are? is case the to-be-transformed source, baseline the expected transformation result, and result the real one?

and is it common to define those apart from each other? i’ve also seen structures like:

cases-of-a-type
├ case-1
│ ├ source.in
│ ├ expected.out
│ └ result.out
┆

If that's what you are looking for, then the only skip failing may not really apply because the test suite is going through all case folders for you. So can can't only or skip particular one of them.

this is true. the three ways i can come up with to do this are all flawed:

  1. a special marker file in the fixture’s case folder (not possible for all cases: it could be a case file instead of a folder)
  2. a component of its file/folder name (rename foo.infoo.only.in. would require the user to care about handling renames by maybe adjusting the glob used to capture files, and may be surprising if a fixture is named like this by chance)
  3. an option or method call (fixture(ava, paths, { skip: 'foo' }, (t,d) => {…}), fixture.skip('foo')(…). both a bit unwieldy for quick adding/removal, and decoupled from the cases)

and i can’t find something on the internet about this

unional commented 7 years ago

could you please shortly explain what case and baseline are? is case the to-be-transformed source, baseline the expected transformation result, and result the real one?

and is it common to define those apart from each other? i’ve also seen structures like:

Yes. And they are better kept in separate folders, as described in the readme: https://github.com/unional/ava-fixture#usage

One benefit is to commit cases and baselines to git while ignoring the results folder.

Also, it is more flexible because your code may need to work on multiple files, or even folders. One example is https://github.com/unional/jspm-config

Doing a full automated test like you are looking for definitely has it merit, but you also lost the flexibility and control.

a special marker file in the fixture’s case folder (not possible for all cases: it could be a case file instead of a folder) a component of its file/folder name (rename foo.in → foo.only.in. would require the user to care about handling renames by maybe adjusting the glob used to capture files, and may be surprising if a fixture is named like this by chance)

These post special restriction to the usage and should not be handled or instructed by the library.

an option or method call (fixture(ava, paths, { skip: 'foo' }, (t,d) => {…}), fixture.skip('foo')(…). both a bit unwieldy for quick adding/removal, and decoupled from the cases)

Maybe, and passing in a predicate to filter out the cases would be an option too. But maybe there is a better API.