Running pnpm with Satellite in the repo breaks tests because of dependency issue

[RC-Lee]

What happened:
I am trying to update ElasticSearch to 8.1.0. for Satellite.
The idea is to get Satellite updated first, do a release, and then have the Telescope Services try out the new Satellite version.
I updated ES and ES-mock for Satellite in #3187, and the tests for Satellite pass.
However, Search tests are now not passing.

What should have happened:
Updating inside of Satellite shouldn't affect other Services.

How to reproduce it (as precise as possible):
Try out #3187

Anything else we need to know?:
@manekenpix and I spent a lot of time trying to figure this out.
There is a hack to get this working locally that I'd like to explain first.

The hack:

1. While working on the branch in Update Satellite to ElasticSearch 8.1.0 #3187, pnpm clear to get rid of all the node_modules in the root and in all services. If pnpm clear doesn't work then manually delete all the node_modules
2. Inside of pnpm-workspace.yaml remove the Satellite related lines, or change the file to below

# pnpm workspace definition: https://pnpm.io/pnpm-workspace_yaml
packages:
  # docusaurus web app
  - 'src/docs'
  # web front-end
  - 'src/web'
  # microservices
  - 'src/api/*'
  # autodeployment
  - 'tools/*'

3. In root run pnpm i
4. head to the Satellite directory, then run npm i please note there's no beginning p
5. In the Satellite jest.config take out moduleDirectories: ['<rootDir>/src/satellite/node_modules', 'node_modules'],
6. Voila, both Satellite and Search tests will now work. In root run pnpm test satellite, pnpm test search, pnpm test, and everything should pass.
7. This is only a local hack, because its not possible to do this in CI

The problem without using the hack is that pnpm tries to be very helpful when installing dependencies. After the merge of Satellite to Telescope, Search (as an example) in pnpm-lock.yaml looks like this

  src/api/search:
    specifiers:
      '@senecacdot/satellite': ^1.26.0
      express-validator: 6.14.0
      nodemon: 2.0.15
    dependencies:
      '@senecacdot/satellite': link:../../satellite
      express-validator: 6.14.0
    devDependencies:
      nodemon: 2.0.15

It seems that when running pnpm install, pnpm links the Services that are using Satellite to the local Satellite, instead of the Satellite package in node_modules

So what's happening in the Search tests specifically now is that the test file is trying to use the ElasticSearch-mock that its using right now. But, the client in the folders outside of the test file is using the locally updated Satellite. This causes the current test fail because the old ES-mock and the new ElasticSearch aren't compatible. But since I was only updating Satellite, and haven't touched anything else in Telescope, this shouldn't be happening.

This will not only affect Search, and should affect our tests anytime there will be major updates in Satellite in the future.
If we want to keep the idea of keeping Satellite as its own app, releasing updates, and then using the new versions in Telescope services, then we'll have to find a way to deal with this issue.

[humphd]

I think we have to stop running tests globally, and start doing it per sub-project. Each project needs its own test setup vs. a giant set of tests, and turbo repo can figure out running them all.

[RC-Lee]

Yes, but the issue is the Search service is currently affected by the local Satellite, which in turn is affecting the tests.

[RC-Lee]

In a case where Satellite wasn't merged inside of Telescope, The Search service would be using the Satellite package (in this case v1.26.0). If we had updated Satellite and made a release (say 1.27.0) in this case, Search wouldn't be affected until we update Telescope to use v1.27.0.
Right now its ignoring the Satellite packages and just using the local Satellite

[humphd]

I wonder if this is related to pnpm, and how Jest is doing module resolution?

[RC-Lee]

I wonder if this is related to pnpm, and how Jest is doing module resolution?

Yeah, pnpm is definitely doing something. The "hack" works because it makes the node_modules in Satellite get unnoticed from pnpm, so pnpm will use the Satellite packages.

[humphd]

I don't full understand this, but I see others discussing issues like this and talking about modifying the way that pnpm does its hoisting, see https://pnpm.io/npmrc#dependency-hoisting-settings.

What I think is happening to you is that the packages are being hoisted into the top-level node_modules where Jest is able to find and resolve them, when it really shouldn't.

[cindyorangis]

In a case where Satellite wasn't merged inside of Telescope, The Search service would be using the Satellite package (in this case v1.26.0). If we had updated Satellite and made a release (say 1.27.0) in this case, Search wouldn't be affected until we update Telescope to use v1.27.0. Right now its ignoring the Satellite packages and just using the local Satellite

If the tests are breaking because we're using a local version of Satellite for Search, can we just force Search to use Satellite from the npm registry? That way it will only use v1.26.0 release tag

I pulled this branch and changed the following in src/api/search/package.json
from

    "@senecacdot/satellite": "^1.26.0",

to

    "@senecacdot/satellite": "git://github.com/Seneca-CDOT/satellite.git#v1.26.0",

I ran pnpm test satellite search and all the tests passed

Test Suites: 2 passed, 2 total
Tests:       66 passed, 66 total
Snapshots:   0 total
Time:        6.619 s

[cindyorangis]

If I also update @elastic/elasticsearch to 8.1.0 and @elastic/elasticsearch-mock to 2.0.0 in root. I get a different set of tests not passing

PS C:\Users\lecin\Documents\repo\telescope> pnpm test search

> @senecacdot/telescope@2.8.0 test C:\Users\lecin\Documents\repo\telescope
> pnpm jest "search"


> @senecacdot/telescope@2.8.0 jest C:\Users\lecin\Documents\repo\telescope
> jest -c jest.config.js -- "search"

LOG_LEVEL set to `silent`, change in jest.config.base.js if you want more logs
"next/jest" is currently experimental. https://nextjs.org/docs/messages/experimental-jest-transformer
info  - Loaded env from C:\Users\lecin\Documents\repo\telescope\.env
jest-haste-map: duplicate manual mock found: indexer
  The following files share their name; please delete one of them:
    * <rootDir>\src\backend\utils\__mocks__\indexer.js
    * <rootDir>\src\api\parser\src\utils\__mocks__\indexer.js

  console.log
    ****************************************
    The logs are in slient mode.
    To change it, go to ../jest-setup.js
    **************************************

      at Object.<anonymous> (src/api/search/test/query.test.js:7:9)

 FAIL  src/api/search/test/query.test.js
  query routers
    Test queries that do not pass validation
      √ Return error 400 if missing all 3 of "post", "author", and "title" param (131 ms)
      √ Return error 400 if "post" param is empty (8 ms)
      √ Return error 400 if "post" param has a length greater than 256 (10 ms)
      √ Return error 400 if "author" param is empty (13 ms)
      √ Return error 400 if "author" param does not have a length between 2 and 256 (28 ms)
      √ Return error 400 if "title" param is empty (11 ms)
      √ Return error 400 if "title" param does not have a length between 2 and 256 (18 ms)
      √ Return error 400 if "to" param has an invalid date format (21 ms)
      √ Return error 400 if "from" param has an invalid date format (19 ms)
      √ Return error 400 if "perPage" param is not an integer between 1 to 10 (43 ms)
      √ Return error 400 if "page" param is not an integer between 0 to 999 (35 ms)
    Test queries that pass validation
      × Search results of more than 0 return "id" and "url" of posts (8 ms)
      × Return error 503 if "hits" returned from ElasticSearch is undefined (11 ms)
      Return status 200 if params pass validation checks
        × Invalid "post" with valid "author" or "title" param should pass validation (22 ms)
        × Invalid "author" with valid "post" or "title" param should pass validation (13 ms)
        × Invalid "title" with valid "author" or "post" param should pass validation (12 ms)

  ● query routers › Test queries that pass validation › Return status 200 if params pass validation checks › Invalid "post" with valid "author" or "title" param should pass validation 

    expect(received).toBe(expected) // Object.is equality

    Expected: 200
    Received: 404

      303 |       it(`Invalid "post" with valid "author" or "title" param should pass validation`, async () => {
      304 |         let res = await request(app).get('/').query({ post: '', author: 'Roxanne' });
    > 305 |         expect(res.status).toBe(200);
          |                            ^
      306 |
      307 |         res = await request(app).get('/').query({ post: '', title: 'OSD' });
      308 |         expect(res.status).toBe(200);

      at Object.<anonymous> (src/api/search/test/query.test.js:305:28)

  ● query routers › Test queries that pass validation › Return status 200 if params pass validation checks › Invalid "author" with valid "post" or "title" param should pass validation 

    expect(received).toBe(expected) // Object.is equality

    Expected: 200
    Received: 404

      315 |       it(`Invalid "author" with valid "post" or "title" param should pass validation`, async () => {
      316 |         let res = await request(app).get('/').query({ author: '', post: 'ElasticSearch' });
    > 317 |         expect(res.status).toBe(200);
          |                            ^
      318 |
      319 |         res = await request(app).get('/').query({ author: '', title: 'OSD' });
      320 |         expect(res.status).toBe(200);

      at Object.<anonymous> (src/api/search/test/query.test.js:317:28)

  ● query routers › Test queries that pass validation › Return status 200 if params pass validation checks › Invalid "title" with valid "author" or "post" param should pass validation 

    expect(received).toBe(expected) // Object.is equality

    Expected: 200
    Received: 404

      329 |       it(`Invalid "title" with valid "author" or "post" param should pass validation`, async () => {
      330 |         let res = await request(app).get('/').query({ title: '', post: 'ElasticSearch' });
    > 331 |         expect(res.status).toBe(200);
          |                            ^
      332 |
      333 |         res = await request(app).get('/').query({ title: '', author: 'Roxanne' });
      334 |         expect(res.status).toBe(200);

      at Object.<anonymous> (src/api/search/test/query.test.js:331:28)

  ● query routers › Test queries that pass validation › Search results of more than 0 return "id" and "url" of posts

    expect(received).toBe(expected) // Object.is equality

    Expected: 200
    Received: 404

      370 |       });
      371 |
    > 372 |       expect(res.status).toBe(200);
          |                          ^
      373 |       expect(res.body.results).toBe(2);
      374 |       expect(res.body.values).toStrictEqual([
      375 |         { id: '1111', url: `${POSTS_URL}/1111` },

      at Object.<anonymous> (src/api/search/test/query.test.js:372:26)

  ● query routers › Test queries that pass validation › Return error 503 if "hits" returned from ElasticSearch is undefined

    expect(received).toBe(expected) // Object.is equality

    Expected: 503
    Received: 404

      393 |       const res = await request(app).get('/').query({ post: 'ElasticSearch' });
      394 |
    > 395 |       expect(res.status).toBe(503);
          |                          ^
      396 |     });
      397 |   });
      398 | });

      at Object.<anonymous> (src/api/search/test/query.test.js:395:26)

Test Suites: 1 failed, 1 total
Tests:       5 failed, 11 passed, 16 total
Snapshots:   0 total
Time:        2.82 s, estimated 3 s
Ran all test suites matching /search/i.
 ELIFECYCLE  Command failed with exit code 1.
 ELIFECYCLE  Test failed. See above for more details.

Perhaps we can't update ES and ES-mock in Satellite by itself but in root as well?

[RC-Lee]

Telescope is my first monorepo so I am not familiar with the term. I had thought that as a monorepo, changing anything in Satellite shouldn't have to change anything in the other parts of the repository (until we tell the services to use the new Satellite update). If I had made the same changes to the old Satellite (the one with its own repo) and everything works fine, then ideally, shouldn't it be working too?

I don't think ElasticSearch and ElasticSearch mock is the problem in this issue.
I know of what you did above, I've tried it already and here's what I am pretty sure is happening.
The different errors you're getting is because we haven't updated the indexer and Search to work with ElasticSearch 8.0 yet.
I can totally update ES and ES-mock in both Telescope and Satellite all at once and get it working. #3026 is basically just waiting for the mock to be updated. But I thought doing so this way would defeat the purpose of having Satellite do its own releases, and have Telescope use the new updated Satellite versions each time.

There are also some really weird things going on with jest configs that happens but I can not explain, and I have not fully explored outside of search and satellite tests. But I'm pretty sure is not really related to solving this current issue, and should be an issue on its own.
We don't seem to be directing the tests to be looking at the right node_module.
Thanks to jest.config.base.js, all the tests seem to be reading off of the root node_module
That's why in #3187 I had to add the lines in the Satellite jest.config

  moduleDirectories: ['<rootDir>/src/satellite/node_modules', 'node_modules'],

If I took it out, then Satellite tests won't run, and it'll get the same issue the current search has where the test file is reading from the root node_module with the old ES and ES-mock versions. Having the the <rootDir>/src/satellite/node_modules will tell the test files to look at the Satellite node_module
However, then we have a second maybe issue. I can't fully take off the second node_modules in this line, because createError will have problems.

To make things even weirder, you can try adding the same line to the Search jest.config.js

  moduleDirectories: ['<rootDir>/src/satellite/node_modules', 'node_modules'],

Basically, telling Search tests to read from the Satellite node_modules
We'll get the same different errors that you have above now, without needing to update ES and ES-mock in root.

[humphd]

Jest doing this module resolution with pnpm's flat node_modules is what's doing this, right? If we stop doing top-level Jest runs, and move to per-sub project like eslint, this goes away?

[RC-Lee]

Jest doing this module resolution with pnpm's flat node_modules is what's doing this, right? If we stop doing top-level Jest runs, and move to per-sub project like eslint, this goes away?

Uh, I think the jest config is another issue that was just exposed by this issue.
I don't think the problem for this issue is with jest. The issue is the Search service is using the local Satellite and not the Satellite package it was supposed to use.

[humphd]

In tests or in general? That is, does Search do this when run normally, or only in Jest?

[RC-Lee]

I tested Search service locally. It fails on my branch. So in general and in tests.

[humphd]

OK, excellent, thank you for testing. Very odd.