SYS-332, SYS-375, SYS-376, SYS-377, SYS-378: Add validateJoinRequest.test.ts

[muni-corn]

Adds unit tests for testing validateJoinRequest and some of its components.

Refactors were required in order to eliminate any circular dependencies that prevented mock functions from working as intended.

To see this work, just run npx jest "validateJoinRequest".

[github-actions]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review Possible Bug The function validateJoinRequestHost in validate.ts might incorrectly handle IP validation when bogons are allowed. It checks for reserved IPs even if bogons are allowed, which might not be the intended behavior. Consider revising the logic to ensure it aligns with the intended use cases. Redundant Code The function verifyUnseen in validate.ts adds public keys to the seen set even if they are already in it. This might lead to redundant operations and could be optimized. Error Handling The function validateJoinRequestHost catches exceptions but does not log them, which could hinder debugging. Consider adding logging for exceptions.

[chrypnotoad]

Summary of all failing tests
FAIL test/unit/src/validateJoinRequest.test.ts (21.152 s)
  ● verifyJoinRequestSigner › should fail with wrong signature owner

    expect(received).toStrictEqual(expected) // deep equality

    Expected: {"fatal": true, "reason": "Bad signature, sign owner and node attempted joining mismatched", "success": false}
    Received: null

      124 |         expect(result).toBeNull()
      125 |       } else {
    > 126 |         expect(result).toStrictEqual(test.expectedError)
          |                        ^
      127 |       }
      128 |     })
      129 |   }

      at Object.<anonymous> (test/unit/src/validateJoinRequest.test.ts:126:24)

  ● verifyJoinRequestSigner › should fail with wrong node public key

    expect(received).toStrictEqual(expected) // deep equality

    Expected: {"fatal": true, "reason": "Bad signature, sign owner and node attempted joining mismatched", "success": false}
    Received: null

      124 |         expect(result).toBeNull()
      125 |       } else {
    > 126 |         expect(result).toStrictEqual(test.expectedError)
          |                        ^
      127 |       }
      128 |     })
      129 |   }

      at Object.<anonymous> (test/unit/src/validateJoinRequest.test.ts:126:24)

two of the new tests failed in the PR check
https://github.com/shardeum/shardus-core/actions/runs/10410903643/job/28833708776?pr=244

[magonjr]

PR LGTM
two tests are failing for me
https://github.com/shardeum/shardus-core/pull/244/files#diff-6691c51708538fc5b3d636534da4165888f4c5746c7fce110a0c467bc02733dfR329

[BelfordZ]

Can you point out the circular dependency that existed, and where it was removed?

I noticed you didnt have any commits that add tests without refactoring. Which test was not possible without needing a refactor?

[muni-corn]

Can you point out the circular dependency that existed, and where it was removed?

I noticed you didnt have any commits that add tests without refactoring. Which test was not possible without needing a refactor?

These tests required mocking fields and functions that were all included in the root of the Join module. Because the actual functions we wanted to test were also in the root module, this sort of self-dependency would upset mocking with jest. (Because the functions' actual implementations are imported as-is before jest can replace any calls with mock implementations.)

For example, all tests using logging functions like info, warn, or error fail with their actual implementations because p2pLogger is uninitialized (and I haven't found a decent way of actually initializing it with log4js yet; it felt unnecessary).

It also became impossible to change allowBogon or seen as needed.

So...

• The logging module was created to separate the logging functions so that jest can mock them. Their mocked implementations currently do nothing when they are called. We can add a custom implementation to use e.g. console.log.
• The state module was created along with functions getAllowBogon and getSeen so that we can mock the values of these state variables with jest functions. Some code using these variables has been changed to use the functions instead, so that any mock implementations are used.
• The types module was created to house TypeScript types specific to the Join module so that other (sub)modules don't have to import the entirety of index.ts to use an interface.
• The validate module was created to isolate the functions we want to test. Probably not necessary with logging and state separated, but considerably reduces the size of our Join/index.ts file, which is nice :)

This fixes any tests that depended on allowBogon, seen, info(), warn(), or error() (e.g. the IP address tests). The issue was the self-dependency of the root module. The goal of the refactor was to separate any mocked functions (and their dependencies, if necessary) into their own modules so that the modules can be mocked separately from the root module.

[BelfordZ]

god help us all if we need this semi colon stuff.