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

Plugin @tapjs/nock doesn't work in a NPM workspace #940

Closed
sinedied opened this issue Oct 5, 2023 · 10 comments · Fixed by #942 or #947
Closed

Plugin @tapjs/nock doesn't work in a NPM workspace #940

sinedied opened this issue Oct 5, 2023 · 10 comments · Fixed by #942 or #947

Comments

@sinedied
Copy link

sinedied commented Oct 5, 2023

I'm working on a monorepo with a pretty standard NPM workspace setup (with my modules under packages/*).

When trying to use plugins, it doesn't seem to work. For example, running this from /packages/my-server/:

tap plugin add @tapjs/nock

will create a /packages/my-server/package-lock.json and fail with an error, whereas it should use the root lockfile (and modules instead).

Manually adding the package with npm i -D @tapjs/nock properly adds it to the root /node_modules folder, but if I add it to my .taprc file it's not used.

Running tap plugin list from /packages/my-server/ shows @tapjs/nock in the list, but trying to use t.nock.snapshot() in a test result in an error telling that t.nock is undefined.

Any idea how to solve this?
Thanks

@sinedied
Copy link
Author

sinedied commented Oct 5, 2023

I also tried running tap plugin add @tapjs/nock from the root of the repo:

adding plugins: [ '@tapjs/nock' ]
'@tapjs/nock' does not appear to be a tap plugin. Could not load module with require(). Cannot find module '@tapjs/nock'
Require stack:
- /Users/sinedied/.nvm/versions/node/v18.16.0/lib/node_modules/tap/node_modules/@tapjs/test/test-built
build failed
build failed

Note that my project is setup as an ESM project with "type": "module"

@sinedied
Copy link
Author

sinedied commented Oct 5, 2023

Continuing my investigation, and it seems that there's probably an issue with @tapjs/nock as trying to load the TapNock class directly in the test shows an error "nock plugin not loaded".

@isaacs
Copy link
Member

isaacs commented Oct 5, 2023

(Just to be clear, we talking about nock, not mock, right? @tapjs/mock is a default built-in plugin that's always present and installing/removing it is just a config switch and rebuild.)

  • /Users/sinedied/.nvm/versions/node/v18.16.0/lib/node_modules/tap/node_modules/@tapjs/test/test-built

Ah, tap 18 does not work (at least, not with custom per-project plugins, not currently) as a global install. You have to install it locally as a dev dependency.

When you add a plugin, the following happens:

  • install the plugin module npm install -D {plugin} if it isn't already present in node_modules
  • ensure that the plugin can be loaded by the Test class. <-- This is where it's failing.
  • build the Test class with all the plugins assembled. <-- This would fail, much more badly, if the above step didn't
  • If any of the above fails, and it had to install the dep, then remove the litter.

When you have tap installed as a global, it's installing the @tapjs/nock plugin locally in ./node_modules, which isn't accessible to the globally installed tap in ~/.nvm/versions/node/v18.16.0/lib/node_modules.

Doing it this way ensures that $ tap ... and import 'tap' are in sync. Otherwise, if a plugin added a loader or a config, then the runner wouldn't be able to know about it, you could have different configs loaded in the framework than are loaded in the runner, etc.

I've thought a bit about some ways to be able to have a tap runner and tap install work if they're not the same install, or multiple versions of the built Test class specific to different folders, because it would be really nice to be able to for example have different plugins for different sets of tests. Then you could have your front-end e2e tests load a jsdom/react plugin or something, and your api tests could use nock, etc. But it's a very tricky problem, so at least currently, it's all or nothing.

If the only reason for having a global tap is so that you can run it on the command line, I recommend either adding ./node_modules/.bin to your PATH, or running npm x with no arguments to enter a subshell with the npm paths set up.

I have this function in my .bashrc that I run as part of the PROMPT_COMMAND, it's super convenient:

array_contains_element () {
  local e
  local match="$1"
  shift
  for e; do [[ "$e" == "$match" ]] && return 0; done
  return 1
}

array_uniq () {
  local e
  local u
  u=()
  for e; do
    if ! array_contains_element "$e" "${u[@]}"; then
      u+=("$e")
    fi
  done
  echo "${u[*]}"
}

export NO_NPX_PATH=$PATH
set_npx_path () {
  local p=$(pwd)
  local ifs=$IFS
  export IFS=:
  local ogpath=($PATH)
  local nm
  nm=()
  local d=$(dirname -- "$p")
  while [ "$p" != "$d" ] && [ "$p" != "$HOME" ]; do
    if [ -d "$p/node_modules" ] || [ -f "$p/package.json" ]; then
      nm+=("$p/node_modules/.bin")
    fi
    p=$d
    d=$(dirname -- "$p")
  done
  local newpath=(${nm[*]}:${NO_NPX_PATH[*]})
  local newpathu=($(array_uniq "${newpath[@]}"))
  export IFS=:
  export PATH="${newpathu[*]}"
  export IFS=$ifs
}

# you can run it on each prompt like this:
export PROMPT_COMMAND="set_npx_path; $PROMPT_COMMAND"

If you use zsh or fish, I'm not sure if this would work, but probably something similar is possible.

@sinedied
Copy link
Author

sinedied commented Oct 5, 2023

Yes it's nock 😉

I have installed locally, I only added a global install for my last tests to run the commands without npx and see if I had better success.

@isaacs
Copy link
Member

isaacs commented Oct 5, 2023

Right, but that's what I'm saying, you can't add plugins if tap isn't found in the current project, or it'll try to add the plugin to itself in the global location, and fail to find it when it goes to run the build.

One potential way to avoid the footgun, I suppose, is for the test runner to try to load itself from the cwd if it's not found there? That might work, but it might be a little bit clever?

Anyway, this is what's happening:

$ which tap
/Users/isaacs/.config/nave/installed/18.18.0/bin/tap

$ npm ls tap
gh-940@ /Users/isaacs/dev/tapjs/gh-940
└── tap@18.4.2


$ npm ls -g tap
/Users/isaacs/.config/nave/installed/18.18.0/lib
└── tap@18.4.2


$ tap plugin add @tapjs/nock
adding plugins: [ '@tapjs/nock' ]
installing: [ '@tapjs/nock@3.1.6' ]

added 4 packages in 278ms

62 packages are looking for funding
  run `npm fund` for details
'@tapjs/nock' does not appear to be a tap plugin. Could not load module with require(). Cannot find module '@tapjs/nock'
Require stack:
- /Users/isaacs/.config/nave/installed/18.18.0/lib/node_modules/tap/node_modules/@tapjs/test/test-built
build failed
build failed
attempting to clean up added packages

removed 4 packages in 258ms

62 packages are looking for funding
  run `npm fund` for details

# Failed, because it installed to ./node_modules, but build is happening
# at the tap in global install space.

But running the plugin add command in the local space works:

$ npm exec tap plugin add @tapjs/nock
adding plugins: [ '@tapjs/nock' ]
installing: [ '@tapjs/nock@3.1.6' ]

added 4 packages in 277ms

62 packages are looking for funding
  run `npm fund` for details

> @tapjs/test-built@0.0.0 prepare
> tshy

successfully added plugin(s):
@tapjs/nock

$ node
Welcome to Node.js v18.18.0.
Type ".help" for more information.
> require('tap').nock
[Function: nockMethod] {
  enableNetConnect: [Function: enableNetConnect],
  disableNetConnect: [Function: disableNetConnect],
  snapshot: [Function: snapshot]
}
> // ^^ plugin was added, functionality is there

isaacs added a commit that referenced this issue Oct 5, 2023
This makes the tap runner load itself from the current project install
if it's being run from somewhere else, so that plugins etc all work as
expected and stay in sync.

If that's not possible, and the user is attempting to run plugin
commands using a tap executable from some other location, print a
warning, and the command to try if it fails.

Fix: #940
isaacs added a commit that referenced this issue Oct 5, 2023
This makes the tap runner load itself from the current project install
if it's being run from somewhere else, so that plugins etc all work as
expected and stay in sync.

If that's not possible, and the user is attempting to run plugin
commands using a tap executable from some other location, print a
warning, and the command to try if it fails.

Fix: #940
@sinedied sinedied changed the title Using plugins in NPM workspace Plugin @tapjs/nock doesn't seem to be working Oct 6, 2023
@sinedied
Copy link
Author

sinedied commented Oct 6, 2023

Sorry if that wasn't clear (was pretty tired yesterday):
I have tap installed in my local project. The global CLI install came after my multiple tries to make it work.

Here's some traces, I removed the global tap install for clarity:

~/projects/azure-search-open-ai-javascript/packages/search   tests ±  npm rm -g tap

removed 368 packages in 3s
 ~/projects/azure-search-open-ai-javascript/packages/search   tests  which tap
tap not found
 ✘  ~/projects/azure-search-open-ai-javascript/packages/search   tests  npm exec tap plugin add @tapjs/nock
adding plugins: [ '@tapjs/nock' ]
installing: [ '@tapjs/nock@3.1.6' ]

added 770 packages in 2m

114 packages are looking for funding
  run `npm fund` for details
'@tapjs/nock' does not appear to be a tap plugin. Could not load module with require(). Cannot find module '@tapjs/nock'
Require stack:
- /Users/sinedied/projects/azure-search-open-ai-javascript/node_modules/@tapjs/test/test-built
build failed
build failed
attempting to clean up added packages

removed 25 packages in 875ms

107 packages are looking for funding
  run `npm fund` for details

Doing that creates a packages/search/package-lock.js file and a packages/search/node_modules folder, which is wrong as I'm in a NPM workspace.

Then I removed these and tried manually:

~/projects/azure-search-open-ai-javascript/packages/search   tests  npm i -D @tapjs/nock

and added:

plugin:
  - "@tapjs/nock"

To my packages/search/.taprc file.

The plugin is recognized with npm exec tap plugin list (last one):

~/projects/azure-search-open-ai-javascript/packages/search   tests ±  npm exec tap plugin list           
@tapjs/after
@tapjs/after-each
@tapjs/asserts
@tapjs/before
@tapjs/before-each
@tapjs/filter
@tapjs/fixture
@tapjs/intercept
@tapjs/mock
@tapjs/node-serialize
@tapjs/snapshot
@tapjs/spawn
@tapjs/stdin
@tapjs/typescript
@tapjs/worker
@tapjs/nock

But when I run my tests, t.nock is undefined.
Even doing it the manual way with:

import { TapNock } from '@tapjs/nock';
...
t.test('testing', async (t) => {
  const tn = new TapNock(t);
  tn.nock.snapshot();

Throws an error "nock plugin is not loaded"

I hope that's more clear.

@sinedied sinedied changed the title Plugin @tapjs/nock doesn't seem to be working Plugin @tapjs/nock doesn't seem to be working in a NPM workspace Oct 6, 2023
@sinedied
Copy link
Author

sinedied commented Oct 6, 2023

I made a minimal repo to reproduce the issue: https://github.com/sinedied/taptest

  1. git clone https://github.com/sinedied/taptest
  2. npm install
  3. cd packages/test && npm test

I'm now sure that the problem comes from the fact that it's in a NPM workspace:

  • it works when I create a similar project, without the workspace (tap plugin add command and nock plugin loading)
  • once the workspace is set up, tap plugin add @tapjs/nock no longer works (same error as in my previous post). Installing it manually works, but then the nock plugin not loaded error comes up when the test is run.

@sinedied sinedied changed the title Plugin @tapjs/nock doesn't seem to be working in a NPM workspace Plugin @tapjs/nock doesn't work in a NPM workspace Oct 6, 2023
@isaacs isaacs reopened this Oct 6, 2023
@isaacs
Copy link
Member

isaacs commented Oct 6, 2023

Ah, I see. It's getting confused about where to run the install/build, I think. But shouldn't be too hard to work out.

The issue that closed this was a legit bug/footgun, and I think related to this, but yes there's another thing as well.

@isaacs
Copy link
Member

isaacs commented Oct 8, 2023

Ok, so here's what I'm finding, just capturing here for posterity:

  1. This does not work: npm install ; npm test -w packages/test -- --snapshot, because the @tapjs/nock plugin isn't built, even though it does get installed in the root node_modules by virtue of the workspace dependency on it.
  2. This does work: npm i ; npm exec -w packages/test tap build ; npm test -w packages/test -- --snapshot. The build in the workspace folder picks up the config, and links the @tapjs/nock package appropriately. After that's done, the npm test -w packages/test command works as expected.
  3. This also works (from the root): npm exec tap plugin add @tapjs/nock ; npm test -w packages/test -- --snapshot, because it adds the @tapjs/nock as a config at the root, and builds with it. Cruft: it adds this to the root package.json
  4. This does not work (from within the workspace): tap plugin add @tapjs/nock; npm test -- --snapshot. The @tapjs/nock plugin is installed in the workspace node_modules rather than in the location where tap lives.
  5. This does work (from within the workspace): tap build ; npm test -- --snapshot, but, only if the plugin can be found in node_modules, otherwise it fails (as described above, the footgun with global packages).
diff --git a/package.json b/package.json
index a932f4d..0671fbb 100644
--- a/package.json
+++ b/package.json
@@ -5,5 +5,10 @@
   "type": "module",
   "workspaces": [
     "packages/*"
-  ]
+  ],
+  "tap": {
+    "plugin": [
+      "@tapjs/nock"
+    ]
+  }
 }

Since 2 and 5 works, the first part of the solution is:

  • Detect if the tap in use is not built with the plugins listed in the config
  • If not, await build([], config) in @tapjs/run: src/run.ts before executing the test processes. Of course, if the build fails (for example if you changed the config, but without actually installing the deps), then it'll abort at this point, and make you fix it.

Since 3 works, the second part of the solution is:

  • Any time tap plugin add needs to install something, do the installation at the parent of the node_modules folder where tap is installed.

That way, it'll install in the workspace root when you tap plugin add @tapjs/nock within a workspace project, even though the plugin config is actually set in packages/{workspacename}/{package.json,.taprc}

isaacs added a commit to isaacs/resolve-import that referenced this issue Oct 8, 2023
isaacs added a commit that referenced this issue Oct 8, 2023
Need it to realpath symlinked packages, to address the plan
in #940
isaacs added a commit that referenced this issue Oct 8, 2023
build on demand
perform installs in monorepo root, not identical to project dir

just hitting save on it, still needs tests, but working in repro case
isaacs added a commit that referenced this issue Oct 8, 2023
Need it to realpath symlinked packages, to address the plan
in #940
isaacs added a commit that referenced this issue Oct 8, 2023
1. Detect if the tap in use is not built with the plugins listed in the
config. If this is the case, run `build([], config)` prior to spawning
test processes.

2. When executing npm install as part of `tap plugin add`, detect the
appropriate place to do so, in this priority order:

  - the parent of the node_modules folder containing `tap`
  - the parent of the node_modules folder containing `@tapjs/test`
  - the output of `npm prefix` (ie, workspace root)
  - the root of the project (previous behavior)

Adds `t.pluginSignature` getter which returns the built string signature
which can be compared against the desired set captured by
`config.pluginSignature`.

Fix: #940
isaacs added a commit that referenced this issue Oct 8, 2023
1. Detect if the tap in use is not built with the plugins listed in the
config. If this is the case, run `build([], config)` prior to spawning
test processes.

2. When executing npm install as part of `tap plugin add`, detect the
appropriate place to do so, in this priority order:

  - the parent of the node_modules folder containing `tap`
  - the parent of the node_modules folder containing `@tapjs/test`
  - the output of `npm prefix` (ie, workspace root)
  - the root of the project (previous behavior)

Adds `t.pluginSignature` getter which returns the built string signature
which can be compared against the desired set captured by
`config.pluginSignature`.

Fix: #940
@sinedied
Copy link
Author

sinedied commented Oct 9, 2023

Thanks for your time detailing all your investigation and the fix, I didn't expect such a quick fix ❤️

I can confirm that it now works with the latest version!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants