Skip to content
This repository has been archived by the owner on Oct 30, 2022. It is now read-only.

Extract runner from unmock-core #380

Merged
merged 20 commits into from
Feb 11, 2020
Merged

Extract runner from unmock-core #380

merged 20 commits into from
Feb 11, 2020

Conversation

carolstran
Copy link
Contributor

Part of #299

  • Created separate unmock-runner package
  • Made exports more generic to be able to run with Jest, Mocha or any other future imported runner function
  • Added a temporary jestRunner function under unmock-runner to use until we have a proper unmock-jest-runner package

TODO in future PRs:

  • Publish this as a package on NPM
  • Create/publish unmock-jest-runner (to replace packages/unmock-runner/src/jestRunner)
  • Create/publish unmock-mocha-runner
  • Add appropriate documentation and configuration (ie add unmock-runner to lint-ts and lint-ts-fix commands)

@mikesol My build is still failing because it cannot find the unmock-runner module. Assuming this is a lerna thing? Any tips for how to resolve would be appreciated.

Copy link

@mikesol mikesol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff!

One question - from a design perspective, should the jestRunner be only in the __test__ folders?

By including it in src instead of __test__ and exporting it, it means traditionally that we intend for it to be a user-facing API, but given that we'll replace it soon with unmock-jest-runner and probably won't get a chance to document jestRunner, I'm not sure we want it to be part of the src API.

Just a thought, probably a minor detail in the scheme of things as we'll be deleting it soon, but it seems cleaner. Although the disadvantage is that we'd have to copy it to each package that uses it, which having it in one place as you've done solves.

@carolstran
Copy link
Contributor Author

Makes sense @mikesol, although I also think it's something that will soon be replaced (as in the next day or two) so maybe not urgent for the time being.

Any thoughts on the build error though? Because that is blocking for merge.

@mikesol
Copy link

mikesol commented Feb 10, 2020

Agreed that it is bikeshedding.
The most recent build I can see is 3260, and in there, the issue is this line:

import runner, { IMeeshkanDoneCallback, IRunnerOptions } from "unmock-runner";

I think it should be this instead (haven't tested...):

import runner, { IMeeshkanDoneCallback, IRunnerOptions } from "./";

@carolstran
Copy link
Contributor Author

carolstran commented Feb 10, 2020

Everything's passing now! @mikesol, let me know if I should merge because then I can work on the packages tomorrow.

@mikesol
Copy link

mikesol commented Feb 11, 2020

Cool! @ksaaskil can also add his review, after which my recommendation would be the following:

  • merge
  • release
  • build an unmock-jest-runner package that is the jestRunner, test and release a 0.0.0 version on npm
  • delete the jestRunner and release a new unmock-js
  • update the documentation, README.md, koans, unmock-examples and unmock-katacoda to use unmock-jest-runner
  • build an unmock-mocha-runner package. this is the one that IMO is the most perilous, as we have not yet tested if the abstraction in unmock-runner is powerful enough to handle what mocha needs. For example, mocha may have some error validation scheme or return values from tests that require the abstraction in unmock-runner (ie the types) to change. In the ideal case, though, it'll be as short as the jestRunner and everything will work.

Copy link
Contributor

@ksaaskil ksaaskil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really cool! Also my only comment would be to get rid of imports like ../../unmock-runner/src/jestRunner and replace those with something like import { jestRunner } from unmock-runner (if unmock-runner package exports jestRunner, I'm not sure if it does :))

import NodeBackend from "../backend";
const { withCodes } = transform;
import jestRunner from "../../../unmock-runner/src/jestRunner";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't think one should refer to other Lerna packages via "../../../" but with something like import { runner} from "unmock-runner". Jest will figure out that unmock-runner should point to the local package.

@carolstran
Copy link
Contributor Author

@mikesol Will the unmock-runner be accessible via the unmock-js package or do we need to create its own npm package? Asking because some of the types/initial setup from the runner is necessary for unmock-jest-runner

@ksaaskil That's a good point and I do agree, but it's going to be replaced as soon as we implement the unmock-jest-runner (hopefully today or tomorrow). However, if it really bothers you - I can fix it now!

@ksaaskil
Copy link
Contributor

@mikesol Will the unmock-runner be accessible via the unmock-js package or do we need to create its own npm package? Asking because some of the types/initial setup from the runner is necessary for unmock-jest-runner

@ksaaskil That's a good point and I do agree, but it's going to be replaced as soon as we implement the unmock-jest-runner (hopefully today or tomorrow). However, if it really bothers you - I can fix it now!

No reason to fix it now, it was just a comment :)

@mikesol
Copy link

mikesol commented Feb 11, 2020

@mikesol Will the unmock-runner be accessible via the unmock-js package or do we need to create its own npm package? Asking because some of the types/initial setup from the runner is necessary for unmock-jest-runner
@ksaaskil That's a good point and I do agree, but it's going to be replaced as soon as we implement the unmock-jest-runner (hopefully today or tomorrow). However, if it really bothers you - I can fix it now!

unmock-runner will be its own npm package that we'll import from, and it will be published automatically by lerna. The types will be accessible from that package. unmock-js is not a package AFAIK. Does that answer your question?

@carolstran
Copy link
Contributor Author

@mikesol Yes it does, thank you! Didn't realize that lerna publishes packages automatically.

@carolstran carolstran merged commit caaa87c into dev Feb 11, 2020
@carolstran carolstran deleted the mocha branch February 11, 2020 11:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants