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

feat(provider-states): Add beforeEach and afterEach hooks to provider verification #529

Merged
merged 5 commits into from Feb 2, 2021

Conversation

mefellows
Copy link
Member

@mefellows mefellows commented Dec 9, 2020

Adds support for before/after hooks on individual interactions on the verification process.

The before hook is fired before any of the other hooks (state handlers, request filters and the actual verification) and the after hook fires after all of the others have completed.

The full lifecycle is documented here: https://github.com/pact-foundation/pact-js/pull/529/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R516

Fixes #526

@mefellows mefellows marked this pull request as draft December 9, 2020 22:45
@mefellows
Copy link
Member Author

mefellows commented Dec 9, 2020

@TimothyJones I'll add tests for this, because there is a functional impact to the change. Wanted to get it in front of you early to see if you had thoughts about doing this or not.

@TimothyJones
Copy link
Contributor

I think the need for this illustrates that pact verification is best run outside of a test framework ;)

On a serious note, I have two comments:

  1. If we are going to recommend that verification be run (or can be run) by a test framework, perhaps we should (also?) expose something that makes it easy to use the native before/after test hooks (eg, an array of verification promises or similar).

  2. I’m not sure this fixes the linked issue- my read of the feature request is that it is asking for a companion teardown for each state setup method (state => { setup, teardown }), But, unless I’m missing something, this is generic beforeEach/afterEach.

Generally, I like this feature and I think it’s worth including - although I don’t think I would personally use it, it supports more scenarios, and that is definitely an improvement

@mefellows
Copy link
Member Author

I think the need for this illustrates that pact verification is best run outside of a test framework ;)

Haha

If we are going to recommend that verification be run (or can be run) by a test framework, perhaps we should (also?) expose something that makes it easy to use the native before/after test hooks (eg, an array of verification promises or similar).

Yes, that would be nice. Using the json output from the verifier process would allow us to hook onto the results and report differently, but we'd really need to build the hooks into the verifier process itself to achieve what you're requesting (I think).

FWIW, the Golang process does the reporting on an interaction level (because there is really only one test running in the case of Golang. Harder to do with JS because you need an interface for each of the languages).

I’m not sure this fixes the linked issue- my read of the feature request is that it is asking for a companion teardown for each state setup method (state => { setup, teardown }), But, unless I’m missing something, this is generic beforeEach/afterEach.

You're right indeed. I was thinking of the way the Golang implementation worked, not the JVM.

I think, however, for all intents are purposes the same effect can be achieved without the need for companion teardown methods (for clarify, we are referring to this JVM feature). Having separate hooks per state is doable though and we could add those if needed.

I think the key use case here, is resetting a database back to the default. Because the after hook here happens at the end (always), that's doable. I've kept it consistent with the Golang implementation.

But I guess @brendan-donegan is better placed to answer - will this solve your needs?

@brendan-donegan
Copy link

If I understand correctly, the 'after' hook is always going to be the same for each interaction? I might be able to make that work but @TimothyJones is right to say that I was thinking of the way the JVM implementation works.

@TimothyJones TimothyJones marked this pull request as ready for review February 2, 2021 02:22
@TimothyJones
Copy link
Contributor

Update on this: I took a stab at implementing something like

export type Hook = () => Promise<unknown>

export type StateHandlerFunction = 
  | Hook 
  | { setup: () => Promise<unknown>; teardown: () => Promise<unknown> }

export interface StateHandler {
  [name: string]: StateHandlerFunction
}

But unfortunately the Ruby binaries don't let us have clear control over teardown. This will have to wait for V3 :(

I've updated the PR so that we have beforeEach and afterEach (just a rename of matt's code) and we'll release it shortly.

@bethesque
Copy link
Member

I can add a teardown hook if you really want it. It's supported in the Ruby impl, just not exposed in the CLI.

@TimothyJones
Copy link
Contributor

I can add a teardown hook if you really want it. It's supported in the Ruby impl, just not exposed in the CLI.

Yes please! Do you want me to open an issue?

@mefellows
Copy link
Member Author

mefellows commented Feb 2, 2021

I believe a per-state hook is possible in the current implementation. The caveat would be that the proxy in verifier.ts would need to track which state it's currently handling, and only invoke the state teardown for the current state (e.g. a condition in the current spot where the afterEach hook is currently firing).

This implies that requests can't be parallelised for a given mock service - which is already the case anyway. So I think it's a fair assumption to add to the proxy layer.

@bethesque
Copy link
Member

@TimothyJones yeah, put it in the pact-provider-verifier. Though it seems a bit pointless if we're about to kill that code anyway!

@mefellows
Copy link
Member Author

@TimothyJones yeah, put it in the pact-provider-verifier. Though it seems a bit pointless if we're about to kill that code anyway!

It'll be here in 12 months, especially for heavy users that won't be able to migrate to the v3 immediately.

@TimothyJones
Copy link
Contributor

I don't think we're likely to kill it, at least until we have all languages running v3 - people will still want to read/write v2 contracts.

@TimothyJones TimothyJones changed the title feat: add support for verification before/after hooks feat(provider-states): Add beforeEach and afterEach hooks to provider verification Feb 2, 2021
@TimothyJones TimothyJones merged commit 8147042 into master Feb 2, 2021
@mefellows mefellows deleted the feat/before-after-hooks branch July 30, 2022 11:19
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.

Feature: Provider state teardown
4 participants