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 mock API #698

Closed
wants to merge 22 commits into from
Closed

Add mock API #698

wants to merge 22 commits into from

Conversation

ruyadorno
Copy link

This brings into tap the standard mocking system we have been using across the ecosystem of packages from the npm cli which consists into hijacking the require calls from a given module and defining whatever mock we want via something as simple as a key/value object.

It's a very conscious decision to make it a very opinionated API, as stated in https://github.com/tapjs/node-tap#tutti-i-gusti-sono-gusti - focusing only on the pattern that have been the standard way we handle mocks.

It builds on the initial draft work from @nlf (ref: https://gist.github.com/nlf/52ca6adab49e5b3939ba37c7f0fc51c6) and initial brainstorming of such an API with @mikemimik - thanks ❤️

Example

t.test('testing something, t => {
  const myModule = t.mock('../my-module.js', {
    fs: { readFileSync: () => 'foo' }
  })

  t.equal(myModule.bar(), 'foo', 'should receive expected content')
})

ruyadorno and others added 6 commits October 14, 2020 23:04
Building from the work started by @nlf

ref: https://gist.github.com/nlf/52ca6adab49e5b3939ba37c7f0fc51c6

Co-authored-by: nlf <quitlahok@gmail.com>
- Added param checks and throws TypeErrors on unexpected usage
- Simplified a bit some of the logic aroung builtin modules cache
- Fixed some errors introduced while porting the original work from @nlf
Copy link
Member

@isaacs isaacs left a comment

Choose a reason for hiding this comment

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

I like this! Only comment is that the module and the mock keys should be modules relative to the file calling t.mock(), not tap itself. (Basically, should work the same as require-inject in that way, since while it does have a bit of a learning curve, it's pretty easy to grok and harder to get wrong, once it is learned.)

lib/mock.js Outdated Show resolved Hide resolved
@coreyfarrell
Copy link
Member

I'm a bit concerned with having something in tap that is dependent on tested code being CJS. I think a warning that this will not work on ESM modules needs to be added to the documentation.

Another question, what is the benefit of t.mock over packaging this as an auxiliary module where users would require('tap-mock')? Even if tap has t.mock = require('tap-mock') my bias tends towards isolating stuff into smaller modules as I find they get tested better.

Not intending to be a blocker on this but I wanted to raise these points.

@ruyadorno
Copy link
Author

I'm a bit concerned with having something in tap that is dependent on tested code being CJS. I think a warning that this will not work on ESM modules needs to be added to the documentation.

sure! I do want to have esm support too and would be willing to follow up with the work to get it there but I guess esm has a bunch of different discussions such as supporting only dynamic imports, etc and I would love to get t.mock() cjs usable before jumping down that other rabbit hole.

Another question, what is the benefit of t.mock over packaging this as an auxiliary module where users would require('tap-mock')? Even if tap has t.mock = require('tap-mock') my bias tends towards isolating stuff into smaller modules as I find they get tested better.

I guess this is the very opinionated and personal-preference aspect of the developer experience involved around using the test runner, I for one much prefer the DX of having these essential built-in features right there in t and a very good comparison I can think of is the addition of the Fixtures API (t.testdir, t.fixture, etc over require('tacks') and similars) - it has changed SO MUCH for the better my personal usage that it was the turning point for tap to become my default test runner for every new project - all that to say that my contribution to add t.mock() is also in the hope of steering the project a little bit more in favor of this kind of out-of-the-box complete test runner experience, that to a large extent tap already has by providing coverage out of the box, fixtures, snapshots, etc

Hope that makes sense to you @coreyfarrell and thanks, I appreciate your feedback 😄

@isaacs
Copy link
Member

isaacs commented Oct 22, 2020

Here's how to get the filename of the file that's calling t.mock() so that all the paths can be resolved from there:

// in lib/test.js
class Test {
  // ....
  mock (module, mocks) {
    const {file} = stack.at(Test.prototype.mock)
    const resolved = path.resolve(file)
    // console.error(resolved)
    // then resolve module paths from there, pass to the Mock object, etc.
  }
  // ....
}

EDIT: updated because I noticed that stack-utils has a helper method that does all of this, in a slightly more efficient way, setting the stackTraceLimit to 1 since we only need a single frame.

The mock keys should now be paths relative to the current script/tests
that is defining it.
@ruyadorno
Copy link
Author

hey @isaacs I took the time to implement the suggested changes and clean things up over the weekend - also added many variations of the scenarios we chatted about to make sure module resolution works as intended in test/test.js 😊

Add support to modules that instant executing a required module.

    require('./something')()

This mock-fn-as-callback-style was used in test found in npm/cli:

    t.test('mock as callback style', t => {
      t.mock('../my-module.js', {
        '../sub-module.js': arg => {
          t.equal(arg, 'expected')
          t.end()
        }
      })
    })

Also added more varied situations to stress test the module replacement,
such as using t.mock within one of the defined mocks, require calls at
execution time along with tests for the mock as callback style.
@ruyadorno
Copy link
Author

ruyadorno commented Nov 17, 2020

[update] I'm in the process of replacing all mock usage within the https://github.com/npm/cli codebase with this t.mock implementation and during that process I found some very specific edge cases which I'm currently tackling - I'll make sure to add a ref between the PR with the rewrote mocks for npm and this one once that's ready to go. 😊

@ruyadorno ruyadorno force-pushed the add-mock-api branch 2 times, most recently from cafcec4 to 6c5cb81 Compare December 17, 2020 22:57
@ruyadorno
Copy link
Author

Here's the PR that updates the npm cli to use t.mock instead: npm/cli#2370

isaacs pushed a commit to tapjs/libtap that referenced this pull request Feb 16, 2021
This brings into **tap** the standard mocking system we have been using
across the ecosystem of packages from the npm cli which consists into
hijacking the `require` calls from a given module and defining whatever
mock we want via something as simple as a key/value object.

It's a very conscious decision to make it a very opinionated API, as
stated in https://github.com/tapjs/node-tap#tutti-i-gusti-sono-gusti -
focusing only on the pattern that have been the standard way we handle
mocks.

It builds on the initial draft work from @nlf (ref:
https://gist.github.com/nlf/52ca6adab49e5b3939ba37c7f0fc51c6) and
initial brainstorming of such an API with @mikemimik - thanks ❤️

Example:

```js
t.test('testing something, t => {
  const myModule = t.mock('../my-module.js', {
    fs: { readFileSync: () => 'foo' }
  })

  t.equal(myModule.bar(), 'foo', 'should receive expected content')
})
```

Credit: @ruyadorno, @nlf
Reviewed-by: @isaacs
PR-URL: tapjs/tapjs#698
Closes: tapjs/tapjs#698
isaacs pushed a commit to tapjs/libtap that referenced this pull request Feb 16, 2021
This brings into **tap** the standard mocking system we have been using
across the ecosystem of packages from the npm cli which consists into
hijacking the `require` calls from a given module and defining whatever
mock we want via something as simple as a key/value object.

It's a very conscious decision to make it a very opinionated API, as
stated in https://github.com/tapjs/node-tap#tutti-i-gusti-sono-gusti -
focusing only on the pattern that have been the standard way we handle
mocks.

It builds on the initial draft work from @nlf (ref:
https://gist.github.com/nlf/52ca6adab49e5b3939ba37c7f0fc51c6) and
initial brainstorming of such an API with @mikemimik - thanks ❤️

Example:

```js
t.test('testing something, t => {
  const myModule = t.mock('../my-module.js', {
    fs: { readFileSync: () => 'foo' }
  })

  t.equal(myModule.bar(), 'foo', 'should receive expected content')
})
```

Credit: @ruyadorno, @nlf
Reviewed-by: @isaacs
PR-URL: tapjs/tapjs#698
Closes: tapjs/tapjs#698
coreyfarrell pushed a commit to tapjs/libtap that referenced this pull request Feb 16, 2021
This brings into **tap** the standard mocking system we have been using
across the ecosystem of packages from the npm cli which consists into
hijacking the `require` calls from a given module and defining whatever
mock we want via something as simple as a key/value object.

It's a very conscious decision to make it a very opinionated API, as
stated in https://github.com/tapjs/node-tap#tutti-i-gusti-sono-gusti -
focusing only on the pattern that have been the standard way we handle
mocks.

It builds on the initial draft work from @nlf (ref:
https://gist.github.com/nlf/52ca6adab49e5b3939ba37c7f0fc51c6) and
initial brainstorming of such an API with @mikemimik - thanks ❤️

Example:

```js
t.test('testing something, t => {
  const myModule = t.mock('../my-module.js', {
    fs: { readFileSync: () => 'foo' }
  })

  t.equal(myModule.bar(), 'foo', 'should receive expected content')
})
```

Credit: @ruyadorno, @nlf
Reviewed-by: @isaacs
PR-URL: tapjs/tapjs#698
Closes: tapjs/tapjs#698
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.

None yet

3 participants