Feat/be/network service tests #983
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
Server/index.js (2)
24-28: Organize Import Statements for ClarityConsider grouping and ordering your import statements to enhance readability. Typically, imports are organized in the following order: built-in modules, third-party modules, and local modules.
157-157: Evaluate Necessity of Passed DependenciesReview whether all the dependencies passed to
NetworkService(axios,ping,logger,http) are necessary. If any of them aren't used withinNetworkService, consider removing them to keep the codebase clean.Server/service/networkService.js (1)
336-336: Provide a descriptive error message when throwing an error.Throwing a generic
Errorwithout a message can make debugging more difficult. Including a descriptive message aids in troubleshooting.Apply this diff:
- throw new Error(); + throw new Error("Failed to insert check into the database.");Server/tests/services/networkService.test.js (2)
248-256: Don't let this slip: Ensure the test fails if no error is thrownIn the test case for
measureResponseTime, ifmockOperationdoesn't throw an error, the test will pass silently. To make sure we capture any unexpected behaviours, add an assertion to fail the test if no error occurs. Seize this chance to make your tests robust.Apply this diff to enhance the test:
try { await networkService.measureResponseTime(mockOperation); + // If we reach this point, the test should fail + expect.fail("Expected an error to be thrown, but none was thrown."); } catch (error) { expect(error).to.have.property("responseTime").that.is.a("number"); expect(error).to.have.property("service", networkService.SERVICE_NAME); expect(error).to.have.property("method", "measureResponseTime"); expect(error.message).to.equal("Operation failed"); }Alternatively, consider using Chai's
assert.throwsor adding thechai-as-promisedpackage to handle asynchronous assertions more elegantly.
337-337: Time to clean up: Remove leftover console.log statementThere's a
console.logon line 337 that's probably leftover from debugging. Let's wipe off any unnecessary logs to keep our tests clean and avoid cluttered output.Apply this diff to remove the console log:
- console.log(logAndStoreCheckStub.getCall(0).args[0]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- Server/index.js (2 hunks)
- Server/service/jobQueue.js (1 hunks)
- Server/service/networkService.js (16 hunks)
- Server/tests/services/networkService.test.js (1 hunks)
🧰 Additional context used
🪛 Biome
Server/service/networkService.js
[error] 127-127: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (2)
Server/index.js (2)
157-157: Verify Integration of New Dependencies in NetworkServiceThe
NetworkServiceis now instantiated with additional dependencies (axios,ping,logger,http). Ensure that theNetworkServiceclass is updated to accept these parameters and that they're properly utilized within its methods.
Line range hint
149-157: Confirm Service Initialization OrderThe order of service creation has been adjusted by instantiating
settingsServicebeforeemailService. Verify that this change doesn't introduce any unintended side effects, especially ifemailServicerelies onsettingsServicebeing fully initialized.
| @@ -98,7 +98,7 @@ class JobQueue { | |||
| }, false); | |||
|
|
|||
| if (!maintenanceWindowActive) { | |||
| const res = await this.networkService.getStatus(job); | |||
| await this.networkService.getStatus(job); | |||
There was a problem hiding this comment.
💡 Codebase verification
Heads up! Removing await could lead to some unexpected hiccups!
Dropping the await means getStatus runs in the background without waiting, which might cause errors to go unnoticed. This could lead to unhandled promise rejections or incomplete job processing. It's crucial to ensure that any critical operations within getStatus are properly handled.
- Server/service/jobQueue.js: Line 101 -
this.networkService.getStatus(job);
🔗 Analysis chain
Yo, hold up! We're droppin' the await like it's hot, but is that the plan?
Listen up, homie! We just yeeted that await outta there faster than mom's spaghetti slidin' off a plate. But check it:
- We're now callin'
getStatuslike it's a drive-by. No waitin', no chillin'. - This might make our job processing flow smoother than a fresh beat, but...
- What if
getStatuswas throwin' down some mad important info or errors? We're just gonna ghost 'em now? - If
getStatuswas messin' with the job or doin' some crucial stuff, we might be leavin' it hangin' mid-verse.
Yo, can you spit some bars about why we're makin' this change? Are we sure we wanna let getStatus do its thing in the background without waitin' for it to finish its rhyme?
Let's run a quick check to see if getStatus is doin' anything we can't afford to miss:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Yo, let's see what this `getStatus` method is all about
rg --type js 'getStatus.*\{' -A 10
Length of output: 1653
Script:
#!/bin/bash
# Inspect the createWorker method in jobQueue.js for dependencies on getStatus
rg --type js 'createWorker.*\{' -A 50 Server/service/jobQueue.js
Length of output: 1441
| //Network service and dependencies | ||
| import NetworkService from "./service/networkService.js"; | ||
| import axios from "axios"; | ||
| import ping from "ping"; | ||
| import http from "http"; |
There was a problem hiding this comment.
Remove Unused Imports to Clean Up Code
If the newly imported modules (axios, ping, http) aren't utilized elsewhere in Server/index.js or within NetworkService, it's best to remove them to reduce clutter and potential confusion.
| @@ -139,7 +163,7 @@ class NetworkService { | |||
| message: errorMessages.PING_CANNOT_RESOLVE, | |||
| responseTime: error.responseTime, | |||
| }; | |||
| return await this.logAndStoreCheck(checkData, this.db.createCheck); | |||
| await this.logAndStoreCheck(checkData, this.db.createCheck); | |||
| } finally { | |||
| this.handleStatusUpdate(job, isAlive); | |||
There was a problem hiding this comment.
Await asynchronous calls to 'handleStatusUpdate' to ensure proper execution.
The method handleStatusUpdate is asynchronous, but it's being called without await. This could lead to unhandled promise rejections or the rest of the code not waiting for its completion.
Apply this diff:
- this.handleStatusUpdate(job, isAlive);
+ await this.handleStatusUpdate(job, isAlive);Also applies to: 214-214, 283-283
| error.service === undefined ? (error.service = this.SERVICE_NAME) : null; | ||
| error.method === undefined ? (error.method = "measureResponseTime") : null; |
There was a problem hiding this comment.
Avoid assignments within expressions for clearer code.
Assigning values within expressions can reduce readability and may lead to unintended side effects. It's better to separate the assignment from the condition.
Apply this diff to fix the issue:
-error.service === undefined ? (error.service = this.SERVICE_NAME) : null;
-error.method === undefined ? (error.method = "measureResponseTime") : null;
+if (error.service === undefined) {
+ error.service = this.SERVICE_NAME;
+}
+if (error.method === undefined) {
+ error.method = "measureResponseTime";
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| error.service === undefined ? (error.service = this.SERVICE_NAME) : null; | |
| error.method === undefined ? (error.method = "measureResponseTime") : null; | |
| if (error.service === undefined) { | |
| error.service = this.SERVICE_NAME; | |
| } | |
| if (error.method === undefined) { | |
| error.method = "measureResponseTime"; | |
| } |
🧰 Tools
🪛 Biome
[error] 127-127: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 128-128: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
| import sinon from "sinon"; | ||
| import NetworkService from "../../service/networkService.js"; | ||
| import { errorMessages, successMessages } from "../../utils/messages.js"; | ||
| import exp from "constants"; |
There was a problem hiding this comment.
Unused import 'exp' from 'constants'
The import statement import exp from "constants"; on line 4 appears unnecessary. Let's shed any unneeded code to keep things tight and avoid confusion.
Apply this diff to remove the unused import:
-import exp from "constants";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import exp from "constants"; |
| writeToDB.rejects(new Error("Database error")); | ||
| await networkService.logAndStoreCheck(data, writeToDB); | ||
| expect(loggerMock.error.calledOnce).to.be.true; | ||
| expect(loggerMock.error.calledWith(`Error writing check for ${data.monitorId}`)).to.be | ||
| .true; | ||
| }); |
There was a problem hiding this comment.
Handle async operations properly in your tests
In the test for logAndStoreCheck, the await keyword is used with a function that doesn't return a promise due to a missing async keyword. This oversight could make the test tremble unexpectedly. Ensure that asynchronous functions are properly defined and handled.
Apply this diff to fix the async function declaration:
-describe("NetworkService - logAndStoreCheck", async () => {
+describe("NetworkService - logAndStoreCheck", () => {And ensure that the test function is marked as async:
-it("should log an error if `writeToDb` throws an error", async () => {
+it("should log an error if `writeToDb` throws an error", async function () {This adjustment ensures that the asynchronous nature of the test is correctly managed.
Committable suggestion was skipped due to low confidence.
This PR refactors NetworkService to inject dependencies, similar to EmailService. It also fixes some minor bugs in the class discovered during testing. It also implements tests