Add context support on before hook

[kugtong33]

Fixes #1579

Create the context object before passing it to the before hook and then clone it, Object.create(context), before passing it the beforeEach hook so that each beforeEach - test - afterEach sequence has its own copy of the context.

[novemberborn]

Great start @kugtong33, thanks for taking this on!

[novemberborn]

Let's be consistent and name this contextRef throughout.

[novemberborn]

We should expose the context to the after hooks as well. This should be the same object as was created by the before hooks.

[novemberborn]

_hasUnskippedTests() depends on allTests having been created. Wrapping the test building in sequenceTests() won't work. Looks like this behavior isn't covered by an integration test though.

I think it's fine to still build beforeHooks and afterHooks. The trick is to not run them if all tests are skipped. We still "run" the skipped tests since we want to show them in the test results, but they don't actually execute.

[kugtong33]

If I created the context above and injected it directly, I think it should still have work as intended, but the after hook got a mutated(mutations from afterEach, beforeEach, tests) context when I ran the tests.

I need to investigate more :D

const context = {context: {}};

const serialTests = new Sequence(this._buildTests(this.tests.serial, context), this.bail);
const concurrentTests = new Concurrent(this._buildTests(this.tests.concurrent, context), this.bail);
const allTests = new Sequence([serialTests, concurrentTests]);

...

if (this._hasUnskippedTests()) {
    const beforeHooks = new Sequence(this._buildHooks(this.hooks.before, null, context));
    const afterHooks = new Sequence(this._buildHooks(this.hooks.after, null, context));
    finalTests = new Sequence([beforeHooks, allTests, afterHooks], true);
} else {
    finalTests = new Sequence([allTests], true);
}

[novemberborn]

It's actually context.context that needs to be (shallowly) cloned. Let's use https://www.npmjs.com/package/lodash.clone.

You'll have to create a context ref for the before and after hooks, and before those hooks have run you need to pass a cloned ref to the beforeEach and afterEach hooks as well as the tests. Something like this should work if you modify the t.context getter and setter.

class ContextRef {
  constructor () {
    this.value = {}
  }

  get () {
    return this.value
  }

  set (newValue) {
    this.value = newValue
  }

  copy () {
    return new LateBinding(this)
  }
}

class LateBinding extends ContextRef {
  constructor (ref) {
    super()
    this.ref = ref
    this.bound = false
  }

  get () {
    if (!this.bound) this.set(clone(this.ref.get()))
    return super.get()
  }

  set (newValue) {
    this.bound = true
    super.set(newValue)
  }
}

The same copied ref should be passed to the beforeEach and afterEach hooks as well as the tests. The first time the context is accessed it'll be "bound" to the value set in the before hooks. You can pass the original ref to the after hooks.

[kugtong33]

@novemberborn , this is quite circular, and gives me an error on XO, I now understand that the contextRef is injected into the Test class then to the ExecutionContext (that is why you pointed me that way), is it necessary that we should create the LateBinding class? Is it possible that we can make the copy() method returns a clone of the class itself?

Instead, when passing it the beforeEach and afterEach hooks, it would look like this

_buildTestWithHooks(test, contextRef) {
    ...
    const context = contextRef ? contextRef.copy() : new ContextRef();
    ....
}

[kugtong33]

@novemberborn , if it was that easy then it should have been a good-for-beginner issue :D, thanks for the detailed review, will work on it

[novemberborn]

if it was that easy then it should have been a good-for-beginner issue :D, thanks for the detailed review, will work on it

I probably spend half an hour thinking about your code and the expected behavior so I wouldn't say it's that easy! 😉

[novemberborn]

@kugtong33 if you could push your best try I'll have a look tomorrow. Don't worry if it doesn't actually work.

[kugtong33]

@novemberborn , sorry for the late update, I tried the suggestion above that is to bound context.context value, I did it in a simple way, create a Context class with only one method clone then pass the bound context to the beforeEach-test-afterEach sequence, also updated the shared context test to allow t.context on after hook.

I used lodash.clonedeep since the test uses an array as an example and I think if we only do a shallow clone, objects/arrays would conflict on each test sequence.

PS: if this gets accepted, can I be the one to update the docs as well? :D

[novemberborn]

I tried the suggestion above that is to bound context.context value, I did it in a simple way, create a Context class with only one method clone then pass the bound context to the beforeEach-test-afterEach sequence, also updated the shared context test to allow t.context on after hook.

👍 Your Context / ContextBind code is effectively the same as the suggestion I made earlier. I do think my version is clearer in showing intent. "Late binding" signals that the context is only fixed when it's accessed. The explicit get() and set() methods make it easier to see what's going on. It does require a change in lib/test.js but IMHO that's an improvement too.

I used lodash.clonedeep since the test uses an array as an example and I think if we only do a shallow clone, objects/arrays would conflict on each test sequence.

It should use lodash.clone. It's easier to explain that only the t.context value itself is cloned, rather than deeper values being changed. They may be fixtures or perhaps tests rely on object identity. Cloning them can have surprising consequences.

You're right that if you have t.context.arr then each test receives the same array. In the end context is for the test author to manage. We're just trying to find the appropriate default behavior.

---

I found the context and hook behavior quite hard to follow from the PR itself so I started editing the code instead. And at that point leaving code comments and making you do the work again seemed unnecessary, so I've pushed up my changes. Hope you don't mind!

I've fixed the merge conflict. Also, since context is now never null some checks could be removed.

PS: if this gets accepted, can I be the one to update the docs as well? :D

Yes please, that'd be great!

[novemberborn]

I find relying on the setter's behavior to be quite surprising. It's not immediately clear that this.bound and this.boundContext are actually updated.

[novemberborn]

@kugtong33 I've just merged #1670 which changes the TypeScript and Flow type definitions. Currently they're hardcoded to set t.context to null in before and after hooks. Could you update them to pass Context instead? You'll have to merge the master branch into this one, and change AfterInterface<null> and BeforeInterface<null> to AfterInterface<Context> and BeforeInterface<Context> in the index.d.ts and index.js.flow files. Thanks!

[kugtong33]

👍 for the clear intent, there was a lot going on when I checked on master and didn't want this discussion to get messed up by the new commits so I left it that way, and thanks for the guidance :)

All looking good. I'm plannig some refactoring so might merge before you have the documentation ready.

[kugtong33]

@novemberborn , 👍 , will work with the documentation on a separate merge request :)