chore(notifications): migrates from jest to vitest#2745
Conversation
📝 WalkthroughWalkthroughMigrate the notifications app test tooling from Jest to Vitest: remove the Jest config, add a Vitest config, update package.json scripts and devDependencies, and convert test files and mocks to Vitest APIs and vitest-mock-extended. Test logic remains unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2745 +/- ##
=======================================
Coverage 52.39% 52.40%
=======================================
Files 1045 1045
Lines 27825 27825
Branches 6343 6325 -18
=======================================
+ Hits 14579 14581 +2
+ Misses 12800 12789 -11
- Partials 446 455 +9
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
ygrishajev
force-pushed
the
feature/notification-tests
branch
2 times, most recently
from
6c57ab4 to
7fdf2e1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/notifications/src/modules/chain/services/tx-events-service/tx-events.service.spec.ts (1)
187-195:⚠️ Potential issue | 🟡 MinorAlign
setupsignature with the required test helper pattern.Add a single inline-typed options parameter to
setupto match the test setup guideline.♻️ Suggested update
- async function setup() { + async function setup({}: {} = {}) {As per coding guidelines: "Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."apps/notifications/src/interfaces/alert-events/handlers/chain-events/chain-events.handler.spec.ts (1)
50-68:⚠️ Potential issue | 🟡 MinorAlign
setupsignature with the required test helper pattern.Add a single inline-typed options parameter to
setupto match the test setup guideline.♻️ Suggested update
- async function setup() { + async function setup({}: {} = {}) {As per coding guidelines: "Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."apps/notifications/src/modules/chain/services/block-message/block-message.service.spec.ts (1)
101-117:⚠️ Potential issue | 🟡 MinorMake
setupfollow the single-parameter/no-return-type convention.Remove the explicit return type and add a single inline-typed options parameter.
♻️ Suggested update
- async function setup(): Promise<{ - module: TestingModule; - service: BlockMessageService; - blockchainClientService: MockProxy<BlockchainClientService>; - blockMessageParserService: MockProxy<BlockMessageParserService>; - }> { + async function setup({}: {} = {}) {As per coding guidelines: "Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."apps/notifications/src/infrastructure/broker/services/state/state.service.spec.ts (1)
52-75:⚠️ Potential issue | 🟡 MinorMake
setupfollow the single-parameter/no-return-type convention.Remove the explicit return type and add a single inline-typed options parameter.
♻️ Suggested update
- async function setup(): Promise<{ - module: TestingModule; - service: StateService; - pgBossHandlerService: MockProxy<PgBossHandlerService>; - boss: MockProxy<PgBoss>; - pg: MockProxy<Pool>; - }> { + async function setup({}: {} = {}) {As per coding guidelines: "Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."apps/notifications/test/functional/balance-alert.spec.ts (1)
144-163:⚠️ Potential issue | 🟡 MinorAlign
setupsignature with the required test helper pattern.Add a single inline-typed options parameter to
setupto match the test setup guideline.♻️ Suggested update
- async function setup() { + async function setup({}: {} = {}) {As per coding guidelines: "Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."apps/notifications/src/modules/chain/services/blockchain-client/blockchain-client.service.spec.ts (1)
52-65:⚠️ Potential issue | 🟡 MinorMake
setupfollow the single-parameter/no-return-type convention.Remove the explicit return type and add a single inline-typed options parameter.
♻️ Suggested update
- async function setup(): Promise<{ - module: TestingModule; - service: BlockchainClientService; - stargateClient: MockProxy<StargateClient>; - }> { + async function setup({}: {} = {}) {As per coding guidelines: "Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."
🤖 Fix all issues with AI agents
In
`@apps/notifications/src/infrastructure/broker/providers/pg-boss/pg-boss.provider.spec.ts`:
- Around line 20-22: Replace the three usages of `any` with concrete types:
change the MockPgBoss factory (symbol: MockPgBoss) to an arrow function with no
parameters instead of `(..._args: any[])`, redefine the `ExecuteSqlFn` type to
use proper typed parameters for `values` and `rows` (e.g., unknown[] or specific
value/row shapes instead of `any[]`), and update the second mock created with
`vi.fn` (the function taking `options?: any`) to use a properly typed optional
parameter (e.g., `options?: PgBossOptions` or `options?: Record<string,
unknown>`). Ensure the new types are imported or declared in the test file so
the mocks and type alias compile without using `any`.
🧹 Nitpick comments (5)
apps/notifications/src/modules/alert/services/deployment/deployment.service.spec.ts (1)
17-17: Consider updating test descriptions to remove "should" prefix.The test descriptions currently use "should" prefix which doesn't align with the coding guidelines. While these lines weren't changed in this PR, since this is a test file migration, it might be a good opportunity to update them.
- it("should return the deployment calculated escrow balance", async () => { + it("returns the deployment calculated escrow balance", async () => {- it("should return null if deployment is closed", async () => { + it("returns null if deployment is closed", async () => {As per coding guidelines: "Use present simple, 3rd person singular for test descriptions without prepending 'should'"
Also applies to: 55-55
apps/notifications/src/modules/notifications/services/analytics/analytics.service.spec.ts (3)
11-12: UseAnalyticsService.nameand remove "should" prefix from test descriptions.Per coding guidelines:
- Root describe should use
AnalyticsService.nameinstead of hardcoded string to enable automated refactoring tools.- Test descriptions should use present simple, 3rd person singular without "should" prefix.
♻️ Proposed fix
-describe("AnalyticsService", () => { - it("should track events when user is sampled", async () => { +describe(AnalyticsService.name, () => { + it("tracks events when user is sampled", async () => {As per coding guidelines: "Use
<Subject>.namein the root describe suite description" and "Use present simple, 3rd person singular for test descriptions without prepending 'should'".
25-39: Refactor to usesetupfunction with configurable sample rate.This test duplicates mock creation instead of using the
setupfunction. Thesetupfunction should accept a parameter to configure the sample rate, allowing all tests to use it consistently.Also, the test description should not start with "should".
♻️ Proposed fix
- it("should not track events when user is not sampled", async () => { - const amplitude = mock<Amplitude>(); - const hasher = mock<Hasher>(); - const configService = mock<ConfigService<Namespaced<"notifications", NotificationEnvConfig>>>(); - const loggerService = mock<LoggerService>(); - - configService.getOrThrow.mockReturnValue("0.0"); - hasher.hash.mockReturnValue(50); // Hash value that would normally be sampled - - const service = new AnalyticsService(amplitude, hasher, configService, loggerService); - - service.track("user123", "email_sent", { recipient_count: 1 }); - - expect(amplitude.track).not.toHaveBeenCalled(); - }); + it("does not track events when user is not sampled", async () => { + const { service, amplitude } = await setup({ sampleRate: "0.0" }); + + service.track("user123", "email_sent", { recipient_count: 1 }); + + expect(amplitude.track).not.toHaveBeenCalled(); + });And update
setupfunction to accept a parameter:- async function setup() { + async function setup({ sampleRate = "1.0" }: { sampleRate?: string } = {}) { const amplitude = mock<Amplitude>(); const hasher = mock<Hasher>(); const configService = mock<ConfigService<Namespaced<"notifications", NotificationEnvConfig>>>(); const loggerService = mock<LoggerService>(); - configService.getOrThrow.mockReturnValue("1.0"); + configService.getOrThrow.mockReturnValue(sampleRate); hasher.hash.mockReturnValue(50); // Mock hash value that will be sampledAs per coding guidelines: "Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must... accept a single parameter with inline type definition".
41-41: Remove "should" prefix from test description.♻️ Proposed fix
- it("should track email_failed events with error details", async () => { + it("tracks email_failed events with error details", async () => {apps/notifications/src/modules/chain/services/chain-events-poller/chain-events-poller.service.spec.ts (1)
141-141: Avoid casting toany.The
StargateClient as anycast violates the coding guideline that prohibits using typeany. Consider creating a proper mock type or using a type assertion to a more specific mock interface.♻️ Suggested fix
- MockProvider(StargateClient as any), + MockProvider(StargateClient as unknown as new () => StargateClient),Alternatively, you could define a proper mock type for
StargateClientthat satisfies the provider requirements.As per coding guidelines: "Never use type
anyor cast to typeany. Always define the proper TypeScript types."
| const MockPgBoss = vi.fn(function (..._args: any[]) { | ||
| return mockInstance; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd apps/notifications/src/infrastructure/broker/providers/pg-boss && head -80 pg-boss.provider.spec.ts | cat -nRepository: akash-network/console
Length of output: 3131
🏁 Script executed:
rg -n '\bany\b' apps/notifications/src/infrastructure/broker/providers/pg-boss/pg-boss.provider.spec.tsRepository: akash-network/console
Length of output: 314
Remove any from Vitest mock signatures and type definitions.
The test file uses any in three locations, which violates the guideline to always define proper TypeScript types:
- Line 20:
vi.fn(function (..._args: any[]))— arguments are unused; simplify to arrow function - Line 53:
type ExecuteSqlFntype usesany[]for bothvaluesandrows—define proper types - Line 59:
vi.fn(function (options?: any))— parameter should be typed
✅ Suggested fix
+ type ExecuteSqlFn = (text: string, values: unknown[]) => Promise<{ rows: unknown[] }>;
+
it("should use pool.query for executeSql", async () => {
const config = generateBrokerConfig();
const pool = mock<Pool>();
const mockQueryResult: QueryResult = {
command: "SELECT",
rowCount: 0,
oid: 0,
rows: [],
fields: []
};
pool.query.mockImplementation(() => Promise.resolve(mockQueryResult));
- type ExecuteSqlFn = (text: string, values: any[]) => Promise<{ rows: any[] }>;
let capturedExecuteSql: ExecuteSqlFn | undefined;
const mockInstance = mock<PgBoss>();
mockInstance.start.mockResolvedValue(mockInstance);
- const MockPgBoss = vi.fn(function (options?: any) {
+ const MockPgBoss = vi.fn(function (options?: { db?: { executeSql?: ExecuteSqlFn } }) {
if (options?.db?.executeSql) {
capturedExecuteSql = options.db.executeSql;
}
return mockInstance;
});- const MockPgBoss = vi.fn(function (..._args: any[]) {
- return mockInstance;
- });
+ const MockPgBoss = vi.fn(() => mockInstance);📝 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.
| const MockPgBoss = vi.fn(function (..._args: any[]) { | |
| return mockInstance; | |
| }); | |
| const MockPgBoss = vi.fn(() => mockInstance); |
🤖 Prompt for AI Agents
In
`@apps/notifications/src/infrastructure/broker/providers/pg-boss/pg-boss.provider.spec.ts`
around lines 20 - 22, Replace the three usages of `any` with concrete types:
change the MockPgBoss factory (symbol: MockPgBoss) to an arrow function with no
parameters instead of `(..._args: any[])`, redefine the `ExecuteSqlFn` type to
use proper typed parameters for `values` and `rows` (e.g., unknown[] or specific
value/row shapes instead of `any[]`), and update the second mock created with
`vi.fn` (the function taking `options?: any`) to use a properly typed optional
parameter (e.g., `options?: PgBossOptions` or `options?: Record<string,
unknown>`). Ensure the new types are imported or declared in the test file so
the mocks and type alias compile without using `any`.
ygrishajev
force-pushed
the
feature/notification-tests
branch
from
7fdf2e1 to
f26a393
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
apps/notifications/src/modules/chain/services/blockchain-client/blockchain-client.service.spec.ts (2)
15-49:⚠️ Potential issue | 🟡 MinorUse present-tense test descriptions without “should”.
Rename the test titles to present simple, 3rd person singular (e.g., “is defined”, “fetches a block by height”, “fetches the latest block when height is 'latest'”).
As per coding guidelines: Use present simple, 3rd person singular for test descriptions without prepending 'should'.♻️ Suggested rename
- it("should be defined", async () => { + it("is defined", async () => { ... - it("should fetch a block by height", async () => { + it("fetches a block by height", async () => { ... - it('should fetch the latest block when height is "latest"', async () => { + it('fetches the latest block when height is "latest"', async () => {
52-56:⚠️ Potential issue | 🟡 MinorAlign
setupsignature with test guidelines.
setupshould accept a single parameter with an inline type definition and omit the explicit return type.
As per coding guidelines: Usesetupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.♻️ Proposed adjustment
- async function setup(): Promise<{ - module: TestingModule; - service: BlockchainClientService; - stargateClient: MockProxy<StargateClient>; - }> { + async function setup({ + stargateClient = mock<StargateClient>() + }: { + stargateClient?: MockProxy<StargateClient>; + } = {}) { const module = await Test.createTestingModule({ - providers: [BlockchainClientService, MockProvider(LoggerService), { provide: StargateClient, useValue: mock<StargateClient>() }] + providers: [BlockchainClientService, MockProvider(LoggerService), { provide: StargateClient, useValue: stargateClient }] }).compile(); return { module, service: module.get<BlockchainClientService>(BlockchainClientService), - stargateClient: module.get<MockProxy<StargateClient>>(StargateClient) + stargateClient: module.get<MockProxy<StargateClient>>(StargateClient) }; }Also applies to: 57-65
apps/notifications/src/interfaces/alert-events/handlers/chain-events/chain-events.handler.spec.ts (2)
20-21:⚠️ Potential issue | 🟡 MinorRename test titles to present simple without “should”.
Suggested update
- it("should log the received event and process alerts", async () => { + it("logs the received event and processes alerts", async () => { @@ - it("should log the received block and process balance alerts", async () => { + it("logs the received block and processes balance alerts", async () => {As per coding guidelines, "Use present simple, 3rd person singular for test descriptions without prepending 'should'".
Also applies to: 35-36
50-50:⚠️ Potential issue | 🟡 MinorMake
setup()accept a single inline-typed parameter.Suggested update
- async function setup() { + async function setup({}: { } = {}) {As per coding guidelines, "The
setupfunction must ... accept a single parameter with inline type definition".apps/notifications/src/modules/alert/services/wallet-balance-alerts/wallet-balance-alerts.service.spec.ts (3)
125-152:⚠️ Potential issue | 🟡 MinorUpdate test descriptions to present-simple without “should”.
These test titles should follow the present-simple convention.
Proposed fix
- it("should not proceed with an already triggered alert", async () => { + it("does not proceed with an already triggered alert", async () => { - it("should log error if alert repository call fails and reject", async () => { + it("logs error if alert repository call fails and rejects", async () => { - it("should log a single alert error", async () => { + it("logs a single alert error", async () => {As per coding guidelines: “Use present simple, 3rd person singular for test descriptions without prepending 'should'.”
Also applies to: 194-201, 213-224
138-152:⚠️ Potential issue | 🟡 MinorRemove the
as anycast when passing alerts.Please keep the type safe here and in the other occurrences in this file.
Proposed fix
- const alerts: WalletBalanceAlertOutput[] = [alert]; - alertRepository.paginateAll.mockImplementation(async options => { - await options.callback(alerts as any); - }); + const alerts: AlertOutput[] = [alert]; + alertRepository.paginateAll.mockImplementation(async options => { + await options.callback(alerts); + });As per coding guidelines: “Never use type
anyor cast to typeany.”
236-256:⚠️ Potential issue | 🟡 MinorAlign
setuphelper signature and typeonMessageexplicitly.The
setupfunction currently violates coding guidelines in three ways: it has an explicit return type (should be inferred), accepts no parameters (should accept one with inline type), andonMessagelacks explicit typing (defaults to implicitany-like Mock). Update as follows:Proposed fix
- async function setup(): Promise<{ - service: WalletBalanceAlertsService; - loggerService: MockProxy<LoggerService>; - alertRepository: MockProxy<AlertRepository>; - conditionsMatcherService: ConditionsMatcherService; - alertMessageService: MockProxy<AlertMessageService>; - balanceHttpService: MockProxy<BalanceHttpService>; - onMessage: Mock; - }> { + async function setup({}: {} = {}) { const module: TestingModule = await Test.createTestingModule({ providers: [ WalletBalanceAlertsService, ConditionsMatcherService, TemplateService, MockProvider(AlertRepository), MockProvider(AlertMessageService), MockProvider(BalanceHttpService), MockProvider(LoggerService) ] }).compile(); - const onMessage = vi.fn(); + const onMessage: Mock<(message: ReturnType<typeof generateAlertMessage>) => void> = vi.fn();
🤖 Fix all issues with AI agents
In
`@apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.ts`:
- Around line 8-10: The handler created with vi.fn() is untyped and yields
implicit any; replace its creation by typing the mock function (e.g., use
vi.fn<[job: ExpectedJobType]>(() => {}) or the actual PgBoss handler signature)
so the handler variable has an explicit parameter/return type; also fix the mock
for pool.connect so its mock matches the overloaded connect signature (adjust
the MockProxy or typing used for the pool mock) to remove the need for the
`@ts-expect-error` suppressions around pool.connect in the test (reference the
handler variable and pool.connect mock setup in broker.service.spec.ts).
- Line 48: The test's handler mock is untyped (const handler = vi.fn()) which
yields Mock<any, any>; change it to a properly typed mock matching the broker
subscribe handler signature by importing the SubscribeHandler (or the concrete
handler type used by BrokerService.subscribe) and typing the mock accordingly
(e.g., create handler as a vi.fn typed to that SubscribeHandler / mocked
function type) so the mock's parameters and return type are type-checked in the
spec.
🧹 Nitpick comments (3)
apps/notifications/src/infrastructure/broker/services/broker-healthz/broker-healthz.service.spec.ts (2)
67-73: Consider moving timer setup into thesetupfunction.Per coding guidelines,
setupfunction should be used instead ofbeforeEach. The fake timer initialization could be integrated intosetupby accepting an options parameter and returning a cleanup callback.♻️ Suggested refactor
describe("getLivenessStatus", () => { - beforeEach(() => { - vi.useFakeTimers(); - }); - - afterEach(() => { - vi.useRealTimers(); - }); - it("returns ok if db is alive", async () => { - const { service, db } = await setup(); + const { service, db, cleanup } = await setup({ useFakeTimers: true }); db.query.mockResolvedValueOnce({} as never); const result = await service.getLivenessStatus(); expect(result.status).toBe("ok"); expect(result.data.postgres).toBe(true); + cleanup(); });And update
setup:- async function setup(): Promise<{...}> { + async function setup({ useFakeTimers = false }: { useFakeTimers?: boolean } = {}) { + if (useFakeTimers) { + vi.useFakeTimers(); + } const module = await Test.createTestingModule({ ... }).compile(); return { ... + cleanup: () => { + if (useFakeTimers) { + vi.useRealTimers(); + } + } }; }As per coding guidelines: "Use
setupfunction instead ofbeforeEachin unit and service level tests".
120-125: Remove explicit return type fromsetupfunction.Per coding guidelines, the
setupfunction should not have a specified return type—it should be inferred by TypeScript.♻️ Suggested refactor
- async function setup(): Promise<{ - module: TestingModule; - service: BrokerHealthzService; - db: MockProxy<Pool>; - stateService: MockProxy<StateService>; - logger: MockProxy<LoggerService>; - }> { + async function setup() {As per coding guidelines: "The
setupfunction... not have a specified return type".apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.ts (1)
63-65: Prefer overload narrowing over@ts-expect-error(apply in both tests).The suppression hides real typing drift if
pgchanges. A typed overload keeps the intent explicit without suppressions.As per coding guidelines, "Don't add unnecessary comments to the code."♻️ Suggested overload narrowing
-import { vi } from "vitest"; +import { vi, type MockedFunction } from "vitest";- // `@ts-expect-error` Pool.connect has overloaded signatures; TS resolves to the callback variant returning void - pool.connect.mockResolvedValue(poolClient); + const connectMock = pool.connect as MockedFunction<() => Promise<PoolClient>>; + connectMock.mockResolvedValue(poolClient);Also applies to: 91-93
| import { vi } from "vitest"; | ||
| import type { MockProxy } from "vitest-mock-extended"; | ||
| import { mock } from "vitest-mock-extended"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check vitest/vitest-mock-extended versions in package manifests and lockfiles
rg '"vitest' -g 'package*.json' -g '*lock*.yaml' -g '*lock*.json' | head -30Repository: akash-network/console
Length of output: 1898
🏁 Script executed:
# Inspect the spec file to see all the changes mentioned in the scratchpad
cd apps/notifications/src/infrastructure/broker/services/broker 2>/dev/null && cat -n broker.service.spec.ts || rg -n 'broker.service.spec.ts' -A 100Repository: akash-network/console
Length of output: 5965
Type the vi.fn() handler to avoid implicit any type.
Line 48 creates a handler with vi.fn() which defaults to Mock<any, any>. Instead, type it explicitly:
const handler = vi.fn<[job: any]>(() => {});or with the expected signature from PgBoss. This aligns with the guideline forbidding any types in .{ts,tsx,js} files.
Additionally, the @ts-expect-error suppressions on lines 63-65 and 91-93 suggest the mock setup for pool.connect doesn't properly match the overloaded signature. Consider typing the mock to resolve the overload correctly, which would eliminate the need for the suppressions.
🤖 Prompt for AI Agents
In
`@apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.ts`
around lines 8 - 10, The handler created with vi.fn() is untyped and yields
implicit any; replace its creation by typing the mock function (e.g., use
vi.fn<[job: ExpectedJobType]>(() => {}) or the actual PgBoss handler signature)
so the handler variable has an explicit parameter/return type; also fix the mock
for pool.connect so its mock matches the overloaded connect signature (adjust
the MockProxy or typing used for the pool mock) to remove the need for the
`@ts-expect-error` suppressions around pool.connect in the test (reference the
handler variable and pool.connect mock setup in broker.service.spec.ts).
| const queueName = `${configService.getOrThrow("broker.APP_NAME")}.${eventName}`; | ||
| const options = { prefetchCount: faker.number.int({ min: 1, max: 5 }) }; | ||
| const handler = jest.fn(); | ||
| const handler = vi.fn(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.ts | sed -n '40,60p'Repository: akash-network/console
Length of output: 1119
🏁 Script executed:
# Find the BrokerService definition to understand the subscribe signature
fd -e ts -e tsx | xargs grep -l "class BrokerService"Repository: akash-network/console
Length of output: 145
🏁 Script executed:
cat -n apps/notifications/src/infrastructure/broker/services/broker/broker.service.ts | head -100Repository: akash-network/console
Length of output: 3288
🏁 Script executed:
# Find the subscribe method signature
rg "subscribe\(" apps/notifications/src/infrastructure/broker/services/broker/broker.service.ts -A 10 -B 2Repository: akash-network/console
Length of output: 504
🏁 Script executed:
head -40 apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.tsRepository: akash-network/console
Length of output: 1535
🏁 Script executed:
# Check if SingleMsgWorkHandler is imported or available in test
rg "SingleMsgWorkHandler|import.*BrokerService" apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.ts -B 2 -A 2Repository: akash-network/console
Length of output: 307
🏁 Script executed:
# Check if SingleMsgWorkHandler is exported and can be imported
rg "export.*SingleMsgWorkHandler" apps/notifications/src/infrastructure/broker/services/broker/broker.service.tsRepository: akash-network/console
Length of output: 152
🏁 Script executed:
# Get the full test file to see the setup function and understand better
wc -l apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.tsRepository: akash-network/console
Length of output: 152
🏁 Script executed:
# Get the full context around the subscribe test including setup function
cat -n apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.ts | sed -n '40,131p'Repository: akash-network/console
Length of output: 4222
🏁 Script executed:
# Verify if PgBoss.Job is properly typed and available in test
rg "import.*PgBoss" apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.tsRepository: akash-network/console
Length of output: 94
🏁 Script executed:
# Check if SingleMsgWorkHandler is exported from broker.service and verify the typing approach
cd apps/notifications/src/infrastructure/broker/services/broker && \
rg "export.*SingleMsgWorkHandler|export.*type SingleMsgWorkHandler" .Repository: akash-network/console
Length of output: 172
🏁 Script executed:
# Let's verify the exact typing approach by checking if it would work correctly
# Check how vi.fn is used elsewhere in the test file with typing
rg "vi\.fn<" apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.tsRepository: akash-network/console
Length of output: 47
🏁 Script executed:
# Check test coding guidelines for similar patterns in other test files
fd "\.spec\.ts$" -x grep -l "vi\.fn<" {} \; | head -5Repository: akash-network/console
Length of output: 47
🏁 Script executed:
# Look for other handlers/callbacks in the test file to see if there's a pattern
grep -n "vi\.fn" apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.tsRepository: akash-network/console
Length of output: 98
🏁 Script executed:
# Check the pg-boss types to understand Job better
rg "interface Job|type Job" --type ts | grep -i "pgboss\|pg-boss" | head -5Repository: akash-network/console
Length of output: 168
🏁 Script executed:
# Let's see if there are other test files that properly type vi.fn with handler/callback parameters
fd "\.spec\.ts$" apps/notifications -x grep -l "Handler" {} \; | head -3Repository: akash-network/console
Length of output: 282
🏁 Script executed:
# Check if we can use SingleMsgWorkHandler directly - see full import in test
head -20 apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.tsRepository: akash-network/console
Length of output: 977
🏁 Script executed:
# Let's verify what happens when we use the suggested typing approach
# Check if SingleMsgWorkHandler needs to be imported
rg "import.*from.*broker\.service" apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.tsRepository: akash-network/console
Length of output: 114
🏁 Script executed:
# Check if there are simpler typing approaches for vi.fn in other test files
rg "vi\.fn.*PgBoss" apps/notifications --type ts -A 2 -B 2 | head -20Repository: akash-network/console
Length of output: 47
🏁 Script executed:
# Verify the exact structure of how SingleMsgWorkHandler should be used
cat apps/notifications/src/infrastructure/broker/services/broker/broker.service.ts | sed -n '10,20p'Repository: akash-network/console
Length of output: 303
Type the handler mock to avoid implicit any.
vi.fn() defaults to Mock<any, any>. Please bind it to the subscribe handler signature to keep the test type-safe.
♻️ Suggested typing
- const handler = vi.fn();
+ type SubscribeHandler = Parameters<BrokerService["subscribe"]>[2];
+ type HandlerArgs = Parameters<SubscribeHandler>;
+ type HandlerReturn = ReturnType<SubscribeHandler>;
+ const handler = vi.fn<HandlerArgs, HandlerReturn>();Per coding guidelines, "Never use type any or cast to type any. Always define the proper TypeScript types."
🤖 Prompt for AI Agents
In
`@apps/notifications/src/infrastructure/broker/services/broker/broker.service.spec.ts`
at line 48, The test's handler mock is untyped (const handler = vi.fn()) which
yields Mock<any, any>; change it to a properly typed mock matching the broker
subscribe handler signature by importing the SubscribeHandler (or the concrete
handler type used by BrokerService.subscribe) and typing the mock accordingly
(e.g., create handler as a vi.fn typed to that SubscribeHandler / mocked
function type) so the mock's parameters and return type are type-checked in the
spec.
f26a393 to
ea78dee
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -7273,29 +7273,28 @@ | |||
| "@swc/cli": "^0.6.0", | |||
There was a problem hiding this comment.
🔄 Carefully review the package-lock.json diff
Resolve the comment if everything is ok
- node_modules/@nestjs/schematics/node_modules/readdirp 4.1.2
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
apps/notifications/src/modules/chain/services/tx-events-service/tx-events.service.spec.ts (2)
15-15:⚠️ Potential issue | 🟡 MinorUpdate the test description to present simple (no “should”).
Line 15 uses “should…”, which conflicts with the test-description convention. Consider:
As per coding guidelines, “Use present simple, 3rd person singular for test descriptions without prepending 'should'”.✅ Suggested fix
- it("should extract certain events from tx logs", async () => { + it("extracts certain events from tx logs", async () => {
188-191:⚠️ Potential issue | 🟡 MinorMake
setupaccept a single inline-typed parameter.The
setuphelper currently takes no parameters; guidelines require a single parameter with an inline type definition. You can also use it to allow dependency overrides:As per coding guidelines, “Use `setup` function instead of `beforeEach` in test files. The `setup` function must be at the bottom of the root `describe` block, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.”✅ Suggested fix
- async function setup() { + async function setup({ cometClient = mock<Comet38Client>() }: { cometClient?: MockProxy<Comet38Client> } = {}) { const module = await Test.createTestingModule({ - providers: [TxEventsService, { provide: Comet38Client, useValue: mock<Comet38Client>() }, MockProvider(LoggerService)] + providers: [TxEventsService, { provide: Comet38Client, useValue: cometClient }, MockProvider(LoggerService)] }).compile();apps/notifications/src/modules/chain/services/blockchain-client/blockchain-client.service.spec.ts (2)
16-50:⚠️ Potential issue | 🟡 MinorUse present-simple test descriptions (no “should”).
This violates the test description guideline. Rename to present simple, 3rd person singular.
As per coding guidelines: Use present simple, 3rd person singular for test descriptions without prepending 'should'.Proposed update
- it("should be defined", async () => { + it("is defined", async () => { @@ - it("should fetch a block by height", async () => { + it("fetches a block by height", async () => { @@ - it('should fetch the latest block when height is "latest"', async () => { + it('fetches the latest block when height is "latest"', async () => {
53-67:⚠️ Potential issue | 🟡 MinorAdjust
setupsignature to meet test helper rules.
setupmust accept a single parameter with an inline type definition and must not specify a return type.
As per coding guidelines: Usesetupfunction instead ofbeforeEach... Thesetupfunction must ... accept a single parameter with inline type definition ... and not have a specified return type.Proposed update
- async function setup(): Promise<{ - module: TestingModule; - service: BlockchainClientService; - stargateClient: MockProxy<StargateClient>; - }> { + async function setup({ + stargateClient = mock<StargateClient>() + }: { stargateClient?: MockProxy<StargateClient> }) { const module = await Test.createTestingModule({ - providers: [BlockchainClientService, MockProvider(LoggerService), { provide: StargateClient, useValue: mock<StargateClient>() }] + providers: [BlockchainClientService, MockProvider(LoggerService), { provide: StargateClient, useValue: stargateClient }] }).compile();apps/notifications/src/modules/alert/services/alert-message/alert-message.service.spec.ts (2)
12-12:⚠️ Potential issue | 🟡 MinorUpdate test descriptions to present simple (no “should”).
Use 3rd‑person present simple for test names to satisfy the test description style rule.✅ Suggested rename
- it("should return an alert payload with prefix", async () => { + it("returns an alert payload with prefix", async () => { ... - it("should an alert payload without prefix", async () => { + it("returns an alert payload without prefix", async () => {As per coding guidelines: “Use present simple, 3rd person singular for test descriptions without prepending 'should'.”
Also applies to: 27-27
43-45:⚠️ Potential issue | 🟡 MinorAdjust
setupsignature to match the required pattern.
The helper should accept a single parameter with an inline type definition and should not declare an explicit return type.✅ Suggested fix
- async function setup(): Promise<{ - service: AlertMessageService; - }> { + async function setup({}: { }): Promise<{ + service: AlertMessageService; + }> {- async function setup(): Promise<{ + async function setup({}: { }) {As per coding guidelines: “Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.”apps/notifications/src/modules/notifications/services/notification-router/notification-router.service.spec.ts (2)
17-17:⚠️ Potential issue | 🟡 MinorUse present-simple test description.
Replace “should send an email” with present simple, 3rd person singular.
Proposed change
- it("should send an email", async () => { + it("sends an email", async () => {As per coding guidelines: “Use present simple, 3rd person singular for test descriptions without prepending 'should'.”
36-40:⚠️ Potential issue | 🟡 MinorMake
setupconform to the required signature.Remove the explicit return type and accept a single inline-typed parameter.
Proposed change
- async function setup(): Promise<{ - service: NotificationRouterService; - emailSenderService: MockProxy<EmailSenderService>; - notificationChannelRepository: MockProxy<NotificationChannelRepository>; - }> { + async function setup(_: {} = {}) {As per coding guidelines: “Use
setupfunction instead ofbeforeEach… Thesetupfunction must … accept a single parameter with inline type definition … and not have a specified return type.”apps/notifications/src/infrastructure/broker/services/state/state.service.spec.ts (2)
13-42:⚠️ Potential issue | 🟡 MinorUpdate test descriptions to present simple (no “should”)
These
itdescriptions don’t match the project’s required grammar. Suggested rewrites below.✍️ Proposed renames
- it("should be defined", async () => { + it("is defined", async () => { @@ - it("should initialize with state 'stopped'", async () => { + it("initializes with state 'stopped'", async () => { @@ - it("should set state to 'active' after bootstrap", async () => { + it("sets state to 'active' after bootstrap", async () => { @@ - it("should stop pgBoss and pg pool and set state to 'stopped' on shutdown", async () => { + it("stops pgBoss and pg pool and sets state to 'stopped' on shutdown", async () => { @@ - it("should update state to 'stopped' when PgBoss emits 'stopped'", async () => { + it("updates state to 'stopped' when PgBoss emits 'stopped'", async () => {As per coding guidelines, Use present simple, 3rd person singular for test descriptions without prepending 'should'.
53-59:⚠️ Potential issue | 🟡 MinorAlign
setupsignature with test setup guidelines
setupshould accept a single parameter with an inline type definition and should not specify a return type.✅ Suggested adjustment
- async function setup(): Promise<{ + async function setup({}: { module: TestingModule; service: StateService; pgBossHandlerService: MockProxy<PgBossHandlerService>; boss: MockProxy<PgBoss>; pg: MockProxy<Pool>; - }> { + } = {}) {As per coding guidelines, Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.
🤖 Fix all issues with AI agents
In
`@apps/notifications/src/infrastructure/broker/services/state/state.service.spec.ts`:
- Around line 4-5: The test currently casts boss.on to jest.Mock which is
invalid for Vitest; update the test to use Vitest's mock utilities by treating
boss as a MockProxy<PgBoss> and use vi.mocked(boss).on (or
vi.mocked(boss).on.mock.calls / .mockImplementation as needed) to inspect or
manipulate calls to boss.on; replace any "jest.Mock" casts with vi.mocked(...)
usages and adjust assertions to read from the vi.mocked(boss).on mock call
history.
🧹 Nitpick comments (10)
apps/notifications/src/infrastructure/broker/providers/db/db.provider.spec.ts (1)
10-11: Describe suite and test description don't follow naming conventions.Per coding guidelines:
- The root describe should use
createPgPoolFactory.nameinstead of the hardcoded string"createPgPool".- Test descriptions should use present simple without "should" prefix.
♻️ Proposed fix
-describe("createPgPool", () => { - it("should create a pool with the correct connection string", async () => { +describe(createPgPoolFactory.name, () => { + it("creates a pool with the correct connection string", async () => {As per coding guidelines: "Use
<Subject>.namein the root describe suite description instead of hardcoded class/service name strings" and "Use present simple, 3rd person singular for test descriptions without prepending 'should'".apps/notifications/test/setup-functional-tests.ts (1)
10-10: Consider whetherconsole.erroris acceptable here.The guideline recommends
LoggerServiceoverconsole.error. However, since this is test infrastructure bootstrap code that executes before the application context exists, usingconsole.errormay be the only viable option. If this is intentional, no change is needed.apps/notifications/test/functional/chain-message-alert.spec.ts (1)
20-20: Consider updating test description to remove "should".As per coding guidelines, test descriptions should use present simple, 3rd person singular without prepending "should".
📝 Suggested change
- it("should send an alert based on conditions", async () => { + it("sends an alert based on conditions", async () => {apps/notifications/test/functional/http-tools.spec.ts (2)
21-21: Consider updating test descriptions to remove "should".As per coding guidelines, test descriptions should use present simple, 3rd person singular without prepending "should".
📝 Suggested changes
- it("should succeed with valid request and response", async () => { + it("succeeds with valid request and response", async () => {- it("should fail with invalid body", async () => { + it("fails with invalid body", async () => {- it("should fail with invalid query", async () => { + it("fails with invalid query", async () => {- it("should return 500 on invalid response schema", async () => { + it("returns 500 on invalid response schema", async () => {Also applies to: 32-32, 41-41, 50-50
63-65: Remove explicit return type from setup function.As per coding guidelines, the
setupfunction should not have a specified return type.♻️ Suggested change
- async function setup(): Promise<{ - app: INestApplication; - logger: MockProxy<LoggerService>; - }> { + async function setup() {apps/notifications/src/lib/rich-error/rich-error.spec.ts (1)
5-5: Consider usingRichError.nameinstead of hardcoded string.This enables automated refactoring tools to find all references when renaming the class. As per coding guidelines: "Use
<Subject>.namein the root describe suite description."This is a pre-existing issue, so feel free to address it in a separate PR if out of scope for this migration.
♻️ Suggested change
-describe("RichError", () => { +describe(RichError.name, () => {apps/notifications/src/infrastructure/db/services/db-healthz/db-healthz.service.spec.ts (2)
48-54: Pre-existing: Consider consolidating timer setup into thesetupfunction.The
beforeEach/afterEachpattern for timers predates this PR, but per coding guidelines, test setup should use asetupfunction. This could be refactored by returning timer control fromsetupand callingvi.useRealTimers()in individual tests or via a cleanup callback.This is a low-priority improvement that can be addressed in a future PR. As per coding guidelines: "Use
setupfunction instead ofbeforeEachin test files."
99-115: Pre-existing: Replaceanywith a proper schema type.The
NodePgDatabase<any>usage on lines 102, 106, and 112 violates the guideline against usingany. Consider defining a minimal schema type or usingunknownif the schema isn't needed for these tests.This is a pre-existing issue not introduced by this migration. As per coding guidelines: "Never use type
anyor cast to typeany. Always define the proper TypeScript types."apps/notifications/src/modules/chain/services/block-message/block-message.service.spec.ts (2)
15-15: Test description should use present simple tense.Per coding guidelines, test descriptions should use present simple, 3rd person singular without prepending "should".
♻️ Suggested fix
- it("should be defined", async () => { + it("is defined", async () => {
102-118: Setup function has explicit return type.Per coding guidelines, the
setupfunction should not have a specified return type to allow TypeScript inference.♻️ Suggested fix
- async function setup(): Promise<{ - module: TestingModule; - service: BlockMessageService; - blockchainClientService: MockProxy<BlockchainClientService>; - blockMessageParserService: MockProxy<BlockMessageParserService>; - }> { + async function setup() {
| import { describe, expect, it } from "vitest"; | ||
| import type { MockProxy } from "vitest-mock-extended"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/notifications/src/infrastructure/broker/services/state/state.service.spec.tsRepository: akash-network/console
Length of output: 3132
🏁 Script executed:
rg "vitest-mock-extended" apps/notifications/ --type ts --type tsx -A 2 | head -50Repository: akash-network/console
Length of output: 92
🏁 Script executed:
rg "vitest-mock-extended" apps/notifications --type ts -A 2 | head -50Repository: akash-network/console
Length of output: 5127
Replace jest.Mock cast with Vitest mock API
Line 45 casts boss.on to jest.Mock, which doesn't exist in Vitest. This will cause a compile/runtime error. Since boss is a MockProxy<PgBoss>, use vi.mocked() to access the mock's call history:
Fix
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, vi } from "vitest";- const stoppedListener = (boss.on as jest.Mock).mock.calls.find(([event]: [string]) => event === "stopped")?.[1] as (() => void) | undefined;
+ const stoppedListener = vi.mocked(boss.on).mock.calls.find(([event]: [string]) => event === "stopped")?.[1] as (() => void) | undefined;🤖 Prompt for AI Agents
In
`@apps/notifications/src/infrastructure/broker/services/state/state.service.spec.ts`
around lines 4 - 5, The test currently casts boss.on to jest.Mock which is
invalid for Vitest; update the test to use Vitest's mock utilities by treating
boss as a MockProxy<PgBoss> and use vi.mocked(boss).on (or
vi.mocked(boss).on.mock.calls / .mockImplementation as needed) to inspect or
manipulate calls to boss.on; replace any "jest.Mock" casts with vi.mocked(...)
usages and adjust assertions to read from the vi.mocked(boss).on mock call
history.
ygrishajev
force-pushed
the
feature/notification-tests
branch
from
ea78dee to
c99d213
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
apps/notifications/src/modules/chain/services/block-message/block-message.service.spec.ts (2)
15-99:⚠️ Potential issue | 🟡 MinorUse present‑simple test titles (remove “should”).
The test descriptions should be present simple, third‑person singular, without “should”.
As per coding guidelines: "Use present simple, 3rd person singular for test descriptions without prepending 'should'."✏️ Suggested renames
- it("should be defined", async () => { + it("is defined", async () => { ... - it("should fetch a block and parse its messages", async () => { + it("fetches a block and parses its messages", async () => { ... - it('should fetch the latest block when height is "latest"', async () => { + it('fetches the latest block when height is "latest"', async () => { ... - it("should filter messages by type when messageTypes is provided", async () => { + it("filters messages by type when messageTypes is provided", async () => { ... - it("should return empty messages array when block has no transactions", async () => { + it("returns empty messages array when block has no transactions", async () => {
102-108:⚠️ Potential issue | 🟡 MinorAdjust
setupsignature to match the test setup guideline.The helper should accept a single parameter with an inline type and must not specify a return type.
As per coding guidelines: "Usesetupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."🛠️ Suggested adjustment
- async function setup(): Promise<{ - module: TestingModule; - service: BlockMessageService; - blockchainClientService: MockProxy<BlockchainClientService>; - blockMessageParserService: MockProxy<BlockMessageParserService>; - }> { + async function setup({}: {} = {}) {apps/notifications/src/modules/chain/services/message-decoder/message-decoder.service.spec.ts (2)
14-20:⚠️ Potential issue | 🟡 MinorUse present-simple test names (no “should”).
Rename descriptions like “should be defined” → “is defined” to match the test naming convention.As per coding guidelines: "Use present simple, 3rd person singular for test descriptions without prepending 'should'".
95-106:⚠️ Potential issue | 🟡 MinorAlign
setupsignature with test conventions.
The helper should accept a single inline-typed parameter and avoid an explicit return type.As per coding guidelines: "Use `setup` function instead of `beforeEach` in test files. The `setup` function must be at the bottom of the root `describe` block, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."♻️ Proposed fix
- async function setup(): Promise<{ - module: TestingModule; - service: MessageDecoderService; - }> { + async function setup({}: {} = {}) {apps/notifications/src/modules/alert/services/deployment-alert/deployment-alert.service.spec.ts (2)
23-27:⚠️ Potential issue | 🟡 MinorUse present-simple test names (no “should”).
E.g., “creates a new alert when it does not exist.”As per coding guidelines: "Use present simple, 3rd person singular for test descriptions without prepending 'should'".
209-225:⚠️ Potential issue | 🟡 MinorAdd a single inline-typed parameter to
setup.
The helper should accept one parameter (with inline type) to match the test setup convention.As per coding guidelines: "Use `setup` function instead of `beforeEach` in test files. The `setup` function must be at the bottom of the root `describe` block, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."♻️ Proposed fix
- async function setup() { + async function setup({}: {} = {}) {apps/notifications/src/infrastructure/broker/services/state/state.service.spec.ts (2)
13-20:⚠️ Potential issue | 🟡 MinorUse present-simple test names (no “should”).
E.g., “is defined”, “initializes with state ‘stopped’”, “sets state to ‘active’ after bootstrap”.As per coding guidelines: "Use present simple, 3rd person singular for test descriptions without prepending 'should'".
53-75:⚠️ Potential issue | 🟡 MinorAlign
setupsignature with test conventions.
Add a single inline-typed parameter and remove the explicit return type.As per coding guidelines: "Use `setup` function instead of `beforeEach` in test files. The `setup` function must be at the bottom of the root `describe` block, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."♻️ Proposed fix
- async function setup(): Promise<{ - module: TestingModule; - service: StateService; - pgBossHandlerService: MockProxy<PgBossHandlerService>; - boss: MockProxy<PgBoss>; - pg: MockProxy<Pool>; - }> { + async function setup({}: {} = {}) {apps/notifications/src/modules/notifications/services/notification-router/notification-router.service.spec.ts (2)
17-18:⚠️ Potential issue | 🟡 MinorUse present-simple test names (no “should”).
E.g., “sends an email”.As per coding guidelines: "Use present simple, 3rd person singular for test descriptions without prepending 'should'".
36-49:⚠️ Potential issue | 🟡 MinorAlign
setupsignature with test conventions.
Remove the explicit return type and add a single inline-typed parameter.As per coding guidelines: "Use `setup` function instead of `beforeEach` in test files. The `setup` function must be at the bottom of the root `describe` block, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."♻️ Proposed fix
- async function setup(): Promise<{ - service: NotificationRouterService; - emailSenderService: MockProxy<EmailSenderService>; - notificationChannelRepository: MockProxy<NotificationChannelRepository>; - }> { + async function setup({}: {} = {}) {apps/notifications/test/functional/balance-alert.spec.ts (2)
24-26:⚠️ Potential issue | 🟡 MinorUse
<Subject>.nameand present-simple test names.
Considerdescribe(ChainEventsHandler.name, ...)and rename to “sends an alert based on conditions.”As per coding guidelines: "Use
<Subject>.namein the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references."
As per coding guidelines: "Use present simple, 3rd person singular for test descriptions without prepending 'should'".
144-162:⚠️ Potential issue | 🟡 MinorAdd a single inline-typed parameter to
setup.
This keeps the functional tests aligned with the shared setup convention.As per coding guidelines: "Use `setup` function instead of `beforeEach` in test files. The `setup` function must be at the bottom of the root `describe` block, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."♻️ Proposed fix
- async function setup() { + async function setup({}: {} = {}) {apps/notifications/src/interfaces/notifications-events/handlers/notification/notification.handler.spec.ts (2)
15-22:⚠️ Potential issue | 🟡 MinorUse present-simple test names (no “should”).
E.g., “is defined”, “sends a notification”.As per coding guidelines: "Use present simple, 3rd person singular for test descriptions without prepending 'should'".
31-44:⚠️ Potential issue | 🟡 MinorAlign
setupsignature with test conventions.
Remove the explicit return type and add a single inline-typed parameter.As per coding guidelines: "Use `setup` function instead of `beforeEach` in test files. The `setup` function must be at the bottom of the root `describe` block, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."♻️ Proposed fix
- async function setup(): Promise<{ - controller: NotificationHandler; - notificationRouter: MockProxy<NotificationRouterService>; - loggerService: MockProxy<LoggerService>; - }> { + async function setup({}: {} = {}) {apps/notifications/test/functional/notification-channel-crud.spec.ts (2)
16-18:⚠️ Potential issue | 🟡 MinorUse
<Subject>.nameand present-simple test names.
Replace the hardcoded suite name (e.g.,describe(NotificationChannelController.name, ...)) and rename the test to “performs all CRUD operations against contact points.”As per coding guidelines: "Use
<Subject>.namein the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references."
As per coding guidelines: "Use present simple, 3rd person singular for test descriptions without prepending 'should'".
100-118:⚠️ Potential issue | 🟡 MinorAlign
setupsignature with test conventions.
Remove the explicit return type and add a single inline-typed parameter.As per coding guidelines: "Use `setup` function instead of `beforeEach` in test files. The `setup` function must be at the bottom of the root `describe` block, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."♻️ Proposed fix
- async function setup(): Promise<{ - app: INestApplication; - }> { + async function setup({}: {} = {}) {
🤖 Fix all issues with AI agents
In `@apps/notifications/src/lib/value-backoff/value-backoff.spec.ts`:
- Around line 6-7: Rename the test descriptions to present simple, 3rd person
singular without the leading "should": replace "should return value immediately
when available on first attempt" with "returns value immediately when available
on first attempt" and similarly update the other test titles referenced (lines
with the strings at ~16-17, 30-31, 45-46, 60-61, 73-74, 86-88, 96-98) to use the
same pattern (e.g., "returns ...", "retries ...", "throws ..." as appropriate)
so every it(...) description is present simple, 3rd person singular and does not
start with "should".
- Line 5: Replace the hardcoded test suite title with the function's .name to
make renames safe: update the root describe that currently uses "valueBackoff"
to use valueBackoff.name instead so the suite references the actual exported
function symbol (valueBackoff) rather than a string literal.
🧹 Nitpick comments (5)
apps/notifications/src/lib/rich-error/rich-error.spec.ts (2)
5-5: Consider usingRichError.nameinstead of hardcoded string.This enables automated refactoring tools to find all references when renaming the class.
♻️ Suggested change
-describe("RichError", () => { +describe(RichError.name, () => {As per coding guidelines: "Use
<Subject>.namein the root describe suite description instead of hardcoded class/service name strings."
7-7: Consider removing "should" prefix from test descriptions.The coding guidelines require using present simple, 3rd person singular without prepending "should". This applies to all test descriptions in this file (lines 7, 12, 17, 22, 30, 35, 41, 49, 63, 71, 80, 88).
♻️ Example changes
- it("should return the same message if input is a string", () => { + it("returns the same message if input is a string", () => {- it("should extract message from Error object", () => { + it("extracts message from Error object", () => {As per coding guidelines: "Use present simple, 3rd person singular for test descriptions without prepending 'should'."
apps/notifications/src/infrastructure/broker/services/broker-healthz/broker-healthz.service.spec.ts (2)
15-15: Optional: Align test description with coding guidelines.The test description uses "should be defined" but the coding guidelines specify using present simple, 3rd person singular without prepending "should".
✨ Suggested change
- it("should be defined", async () => { + it("is defined", async () => {As per coding guidelines: "Use present simple, 3rd person singular for test descriptions without prepending 'should'".
120-126: Optional: Remove explicit return type from setup function.The coding guidelines specify that the
setupfunction should "not have a specified return type" to let TypeScript infer it.✨ Suggested change
- async function setup(): Promise<{ - module: TestingModule; - service: BrokerHealthzService; - db: MockProxy<Pool>; - stateService: MockProxy<StateService>; - logger: MockProxy<LoggerService>; - }> { + async function setup() {As per coding guidelines: "The
setupfunction must [...] not have a specified return type."apps/notifications/src/infrastructure/db/services/db-healthz/db-healthz.service.spec.ts (1)
48-54: RemovebeforeEach/afterEachand handle timers per test.Guidelines require using a
setupfunction instead ofbeforeEach. Consider moving fake-timer setup into each test (or intosetupvia an options param) and restore in afinallyblock.Proposed change (pattern for each liveness test)
- beforeEach(() => { - vi.useFakeTimers(); - }); - - afterEach(() => { - vi.useRealTimers(); - }); - it("returns ok if db is alive", async () => { - const { service, db } = await setup(); - db.execute.mockResolvedValueOnce(mockQueryResult()); - - const result = await service.getLivenessStatus(); - - expect(result.status).toBe("ok"); - expect(result.data.postgres).toBe(true); + vi.useFakeTimers(); + try { + const { service, db } = await setup(); + db.execute.mockResolvedValueOnce(mockQueryResult()); + + const result = await service.getLivenessStatus(); + + expect(result.status).toBe("ok"); + expect(result.data.postgres).toBe(true); + } finally { + vi.useRealTimers(); + } });As per coding guidelines: "Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type."
|
|
||
| import { MESSAGE, valueBackoff } from "./value-backoff"; | ||
|
|
||
| describe("valueBackoff", () => { |
There was a problem hiding this comment.
Use <Subject>.name in the root describe.
This suite should use the function’s .name to keep refactors safe.
🔧 Suggested update
-describe("valueBackoff", () => {
+describe(valueBackoff.name, () => {As per coding guidelines: "Use <Subject>.name in the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references".
📝 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.
| describe("valueBackoff", () => { | |
| describe(valueBackoff.name, () => { |
🤖 Prompt for AI Agents
In `@apps/notifications/src/lib/value-backoff/value-backoff.spec.ts` at line 5,
Replace the hardcoded test suite title with the function's .name to make renames
safe: update the root describe that currently uses "valueBackoff" to use
valueBackoff.name instead so the suite references the actual exported function
symbol (valueBackoff) rather than a string literal.
| it("should return value immediately when available on first attempt", async () => { | ||
| const mockRequest = jest.fn().mockResolvedValue("success"); | ||
| const mockRequest = vi.fn().mockResolvedValue("success"); |
There was a problem hiding this comment.
Rename test descriptions to present simple (no “should”).
Each it description should be present simple, 3rd person singular without “should”.
✍️ Example renames
-it("should return value immediately when available on first attempt", async () => {
+it("returns value immediately when available on first attempt", async () => {-it("should retry until value is available", async () => {
+it("retries until value is available", async () => {As per coding guidelines: "Use present simple, 3rd person singular for test descriptions without prepending 'should'".
Also applies to: 16-17, 30-31, 45-46, 60-61, 73-74, 86-88, 96-98
🤖 Prompt for AI Agents
In `@apps/notifications/src/lib/value-backoff/value-backoff.spec.ts` around lines
6 - 7, Rename the test descriptions to present simple, 3rd person singular
without the leading "should": replace "should return value immediately when
available on first attempt" with "returns value immediately when available on
first attempt" and similarly update the other test titles referenced (lines with
the strings at ~16-17, 30-31, 45-46, 60-61, 73-74, 86-88, 96-98) to use the same
pattern (e.g., "returns ...", "retries ...", "throws ..." as appropriate) so
every it(...) description is present simple, 3rd person singular and does not
start with "should".
2 participants
Summary by CodeRabbit
Chores
Tests