feat/be/settings service tests #1001
Conversation
WalkthroughThe changes in this pull request focus on the integration of a new 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: 4
🧹 Outside diff range and nitpick comments (2)
Server/service/settingsService.js (1)
43-43: Yo, this loadSettings method is spittin' hot fire! 🎤🔥Man, you're droppin' bars with this
this.appSettings.findOne()change! It's smoother than Slim Shady's flow. But hey, let's make this error handling even more lit:Consider tweaking the error handling like this, fam:
- if (!this.settings) { - throw new Error("Settings not found"); - } + if (!dbSettings) { + throw new Error("Settings not found in the database"); + }This way, we're catchin' the real issue if the database comes up empty. It's like adding that perfect beat drop to your track, you feel me?
Server/tests/services/settingsService.test.js (1)
87-87: Eliminate strayconsole.logto keep output cleanThe
console.log(error);statement in your catch block might be a leftover from debugging. Removing it will prevent unintentional logging during test runs.Apply this diff to remove the debug statement:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- Server/index.js (2 hunks)
- Server/service/settingsService.js (2 hunks)
- Server/tests/services/settingsService.test.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
Server/service/settingsService.js (1)
32-33: Yo, this constructor change is fire! 🔥Bro, you just dropped some serious heat with this dependency injection. It's like you're the Eminem of code refactoring! This change is gonna make our testing game stronger than mom's spaghetti.
Server/index.js (1)
37-40: Keep your imports steadyGreat job importing
SettingsServiceandAppSettings. Everything's in place and ready to go, no sweat here.
| @@ -142,7 +144,7 @@ const startApp = async () => { | |||
|
|
|||
| // Create services | |||
| await connectDbAndRunServer(app, db); | |||
| const settingsService = new SettingsService(); | |||
| const settingsService = new SettingsService(AppSettings); | |||
There was a problem hiding this comment.
Don't lose yourself in the initialization order
It looks like settingsService is being instantiated after it's being attached to the request object in the middleware. Let's make sure we've got our services set up before we serve them, so we don't get caught off guard.
Apply this diff to adjust the initialization order:
--- a/Server/index.js
+++ b/Server/index.js
@@ -78,6 +78,14 @@ const startApp = async () => {
req.emailService = emailService;
req.settingsService = settingsService;
next();
});
+ // Initialize services after middleware
+ const settingsService = new SettingsService(AppSettings);
+ await settingsService.loadSettings();
+ const emailService = new EmailService(
+ settingsService,
+ fs,
+ path,
+ compile,
+ mjml2html,
+ nodemailer,
+ logger
+ );
const networkService = new NetworkService(db, emailService, axios, ping, logger, http);
const jobQueue = await JobQueue.createJobQueue(db, networkService, settingsService);Committable suggestion was skipped due to low confidence.
| sandbox.restore(); | ||
| sinon.restore(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up redundant sinon.restore() to keep our tests steady
In the afterEach function, calling both sandbox.restore(); and sinon.restore(); may be unnecessary. Since we're using the sandbox to stub and mock methods, sandbox.restore(); will handle all restorations. Removing sinon.restore(); can prevent redundancy and keep our tests lean.
Apply this diff to remove the redundant call:
Committable suggestion was skipped due to low confidence.
| mockAppSettings = { | ||
| settingOne: 123, | ||
| settingTwo: 456, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize mockAppSettings in beforeEach to avoid heavy side effects
Assigning mockAppSettings inside the beforeEach block ensures it's reset before each test, preventing unintended interactions between tests.
Move the initialization into beforeEach:
...Committable suggestion was skipped due to low confidence.
| try { | ||
| settingsService.getSettings(); | ||
| } catch (error) { | ||
| expect(error.message).to.equal("Settings have not been loaded"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Streamline error testing to capture the moment
You can simplify your error assertion by using Chai's expect(...).to.throw() syntax, making your test more concise and readable.
Refactor the test as follows:
Applying this change seizes the opportunity to make your tests cleaner.
Committable suggestion was skipped due to low confidence.
This PR adds tests for the
SettingsService