Open muni-corn opened 4 weeks ago
โฑ๏ธ 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. |
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
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?
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...
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
.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.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.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.
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"
.