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.

Sign up for GitHub

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

Jump to bottom

test: replace requireInject with t.mock tap api #2370

Closed
wants to merge 2 commits into from
Closed

test: replace requireInject with t.mock tap api #2370

wants to merge 2 commits into from

Conversation

ruyadorno
Copy link
Collaborator

@ruyadorno ruyadorno commented Dec 17, 2020 •
edited

  • Replaces all usage of requireInject with t.mock
  • Standardizes usage of tap as: const t = require('tap')
  • Fixes some rare cases in which mocks were relying in the fact that
    requireInject placed mocks within the require.cache and made mocks
    available for nested files, this type of scenario now requires an extra
    t.mock declaration in order to define mocks to that nested file.
  • Requires a new release of tap, since currently this PR is just relying in a fork to run all tests, ref: Add mock API tapjs/tapjs#698

@ruyadorno ruyadorno requested a review from a team as a code owner December 17, 2020 05:26
@darcyclarke darcyclarke added Release 7.x work is associated with a specific npm 7 release release: next These items should be addressed in the next release labels Dec 17, 2020
@ruyadorno ruyadorno force-pushed the replace-require-inject-with-tap-mock branch 2 times, most recently from 8c01f81 to c50be61 Compare December 17, 2020 19:15
@ruyadorno
test: update to t.mock tap api
fe880f5 
- Replaces all usage of `requireInject` with `t.mock`
- Standardizes usage of tap as: `const t = require('tap')`
- Fixes some minor inconsistencies in tests
@ruyadorno
tap@npm:@ruyadorno/tap-with-mocks@^14.10.8-patch.9
8742c17
@ruyadorno ruyadorno force-pushed the replace-require-inject-with-tap-mock branch from c50be61 to 8742c17 Compare December 17, 2020 23:07
@ruyadorno ruyadorno mentioned this pull request Dec 17, 2020
Closed
darcyclarke
darcyclarke suggested changes Dec 18, 2020
View reviewed changes
Copy link
Contributor

@darcyclarke darcyclarke left a comment •
edited

Choose a reason for hiding this comment

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

Blocked: Need to land the change in tap first & then update the dep in package.json

@darcyclarke darcyclarke removed the release: next These items should be addressed in the next release label Dec 18, 2020
@darcyclarke darcyclarke added Needs Review semver:patch semver patch level for changes labels Jan 6, 2021
@darcyclarke
Copy link
Contributor

@ruyadorno we need to land this. @isaacs I'm willing to commit resources to get this across the finish line next week, unless you've got any reservations about landing it. I know It probably means cutting a minor of tap but I don't know what the blocker has been other than just getting time to do that - maybe we can get more resources dedicated to tap moving forward(?) ~ especially given that we rely on it so heavily throughout the CLI

@darcyclarke darcyclarke added the release: next These items should be addressed in the next release label Feb 13, 2021
@ruyadorno ruyadorno removed the release: next These items should be addressed in the next release label Feb 25, 2021
@ruyadorno
Copy link
Collaborator Author

We need to update to tap@15 (not yet released at this point in time) and do a second pass of updating all test files, fixing conflicts and migrating new instances of require-inject usage.

I'm removing the Release-Next label to better reflect the current state.

@wraithgar
Copy link
Member

Woohoo! This got done in #3069

@wraithgar wraithgar closed this Apr 14, 2021
@ruyadorno
Copy link
Collaborator Author

🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darcyclarke darcyclarke darcyclarke requested changes

Assignees
No one assigned
Labels
Release 7.x work is associated with a specific npm 7 release semver:patch semver patch level for changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ruyadorno @darcyclarke @wraithgar