refactor: moves certs and transaction tests into unit scope#2163
Merged
refactor: moves certs and transaction tests into unit scope#2163
Conversation
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughRefactors certificate flow to inject CertificateManager and AuthService, changes CertificateService.create to accept a userId, adds a DI provider for CertificateManager, adds unit tests for CertificateService and TransactionRepository, removes several functional tests, and updates template-cache tests to use an async file helper. Changes
Sequence Diagram(s)sequenceDiagram
participant Ctrl as CertificateController
participant Auth as AuthService
participant Svc as CertificateService
participant CM as CertificateManager
participant Repo as UserWalletRepository
Note over Ctrl,Svc: New flow uses injected AuthService and CertificateManager
Ctrl->>Auth: read currentUser.id
Auth-->>Ctrl: userId
Ctrl->>Svc: create({ userId })
Svc->>Repo: findOneByUserId(userId)
Repo-->>Svc: userWallet / null
alt wallet found
Svc->>CM: generatePEM(...)
CM-->>Svc: { certPem, pubkeyPem, encryptedKey }
Svc-->>Ctrl: CertificateOutput
else wallet missing / no address
Svc-->>Ctrl: throw NotFound/Error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (2)
apps/api/test/functional/template-cache.spec.ts (2)
17-54: Refactor to usejest-mock-extendedinstead ofjest.mock().The coding guidelines specify that
jest.mock()should not be used in test files. Instead, usejest-mock-extendedto create mocks and pass them as dependencies to the service under test. As per coding guidelines.
62-73: Refactor to use thesetupfunction pattern.The test should follow the
setupfunction pattern as specified in the coding guidelines. Thesetupfunction should create and return the object under test, be placed at the bottom of the rootdescribeblock, and accept a single parameter with inline type definition. As per coding guidelines.Example structure:
describe("Template cache generation", () => { // ... other code ... describe("Generating cache", () => { it("creates files as expected", async () => { const { templateGalleryService } = setup({}); await templateGalleryService.getTemplateGallery(); expect(await readTemplate(`akash-network-awesome-akash-${sha}.json`)).toMatchSnapshot(); expect(await readTemplate(`akash-network-cosmos-omnibus-${sha}.json`)).toMatchSnapshot(); expect(await readTemplate(`cryptoandcoffee-akash-linuxserver-${sha}.json`)).toMatchSnapshot(); }); }); async function readTemplate(filename: string) { return fs.readFile(path.join(__dirname, "../..", "dist/.data/templates", filename), "utf8"); } function setup(options: { githubPAT?: string }) { const templateGalleryService = new TemplateGalleryService({ githubPAT: options.githubPAT ?? env.GITHUB_PAT, dataFolderPath }); return { templateGalleryService }; } });
🧹 Nitpick comments (1)
apps/api/src/transaction/repositories/transaction/transaction.repository.spec.ts (1)
12-17: Consider simplifying the assertion for better readability.The assertion logic is correct but could be more straightforward. Consider:
-expect(transactions).toEqual(expect.arrayContaining(transactionsFound.map(tx => expect.objectContaining({ hash: tx.hash })))); +expect(transactionsFound.length).toBe(10); +transactionsFound.forEach(found => { + expect(transactions).toContainEqual(expect.objectContaining({ hash: found.hash })); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/src/certificate/controllers/certificate.controller.ts(1 hunks)apps/api/src/certificate/providers/certificate-manager.provider.ts(1 hunks)apps/api/src/certificate/services/certificate/certificate.service.spec.ts(1 hunks)apps/api/src/certificate/services/certificate/certificate.service.ts(2 hunks)apps/api/src/transaction/repositories/transaction/transaction.repository.spec.ts(1 hunks)apps/api/test/functional/certificate.spec.ts(0 hunks)apps/api/test/functional/provider-attributes-schema.spec.ts(0 hunks)apps/api/test/functional/template-cache.spec.ts(2 hunks)apps/api/test/functional/transactions.spec.ts(0 hunks)
💤 Files with no reviewable changes (3)
- apps/api/test/functional/provider-attributes-schema.spec.ts
- apps/api/test/functional/transactions.spec.ts
- apps/api/test/functional/certificate.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/certificate/services/certificate/certificate.service.spec.tsapps/api/src/transaction/repositories/transaction/transaction.repository.spec.tsapps/api/test/functional/template-cache.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/certificate/services/certificate/certificate.service.spec.tsapps/api/src/transaction/repositories/transaction/transaction.repository.spec.tsapps/api/test/functional/template-cache.spec.tsapps/api/src/certificate/providers/certificate-manager.provider.tsapps/api/src/certificate/services/certificate/certificate.service.tsapps/api/src/certificate/controllers/certificate.controller.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/certificate/services/certificate/certificate.service.spec.tsapps/api/src/transaction/repositories/transaction/transaction.repository.spec.tsapps/api/test/functional/template-cache.spec.tsapps/api/src/certificate/providers/certificate-manager.provider.tsapps/api/src/certificate/services/certificate/certificate.service.tsapps/api/src/certificate/controllers/certificate.controller.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
📚 Learning: 2025-10-15T16:39:55.348Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 2039
File: apps/deploy-web/tests/ui/change-wallets.spec.ts:4-10
Timestamp: 2025-10-15T16:39:55.348Z
Learning: In the Akash Console E2E tests using the context-with-extension fixture, the first wallet is automatically created during fixture setup via `importWalletToLeap` in `apps/deploy-web/tests/ui/fixture/wallet-setup.ts`, so tests that call `frontPage.createWallet()` are creating a second wallet to test wallet switching functionality.
Applied to files:
apps/api/src/certificate/services/certificate/certificate.service.spec.ts
📚 Learning: 2025-06-08T03:07:13.871Z
Learnt from: stalniy
Repo: akash-network/console PR: 1436
File: apps/api/src/provider/repositories/provider/provider.repository.ts:79-90
Timestamp: 2025-06-08T03:07:13.871Z
Learning: The getProvidersHostUriByAttributes method in apps/api/src/provider/repositories/provider/provider.repository.ts already has comprehensive test coverage in provider.repository.spec.ts, including tests for complex HAVING clause logic with COUNT(*) FILTER (WHERE ...) syntax, signature conditions (anyOf/allOf), and glob pattern matching.
Applied to files:
apps/api/src/transaction/repositories/transaction/transaction.repository.spec.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
Repo: akash-network/console PR: 0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Use `setup` function instead of `beforeEach` in test files
Applied to files:
apps/api/test/functional/template-cache.spec.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
Repo: akash-network/console PR: 0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : `setup` function must be at the bottom of the root `describe` block in test files
Applied to files:
apps/api/test/functional/template-cache.spec.ts
📚 Learning: 2025-07-21T08:24:24.269Z
Learnt from: CR
Repo: akash-network/console PR: 0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-07-21T08:24:24.269Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use `jest.mock()` to mock dependencies in test files. Instead, use `jest-mock-extended` to create mocks and pass mocks as dependencies to the service under test.
Applied to files:
apps/api/test/functional/template-cache.spec.ts
🧬 Code graph analysis (4)
apps/api/src/certificate/services/certificate/certificate.service.spec.ts (1)
apps/api/src/certificate/providers/certificate-manager.provider.ts (1)
CertificateManager(8-8)
apps/api/src/transaction/repositories/transaction/transaction.repository.spec.ts (5)
apps/api/test/seeders/akash-address.seeder.ts (1)
createAkashAddress(4-12)apps/api/test/seeders/akash-message.seeder.ts (1)
createAkashMessage(5-24)apps/api/test/seeders/address-reference.seeder.ts (1)
createAddressReferenceInDatabase(14-16)apps/api/test/seeders/akash-block.seeder.ts (1)
createAkashBlock(7-29)apps/api/test/seeders/transaction.seeder.ts (1)
createTransaction(4-20)
apps/api/src/certificate/services/certificate/certificate.service.ts (1)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(16-20)
apps/api/src/certificate/controllers/certificate.controller.ts (3)
apps/api/src/certificate/services/certificate/certificate.service.ts (1)
singleton(16-42)apps/api/src/auth/services/auth.interceptor.ts (1)
singleton(20-128)apps/api/src/auth/services/auth.service.ts (1)
Protected(43-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (3)
apps/api/test/functional/template-cache.spec.ts (1)
70-72: LGTM: Clean transition to async file operations.The refactor from synchronous to asynchronous file reads is well-executed. The
readTemplatehelper function properly uses the promises API, and the test correctly awaits the results.Also applies to: 76-78
apps/api/src/transaction/repositories/transaction/transaction.repository.spec.ts (1)
1-1: Verify the test setup import aligns with PR objectives.The PR description states the goal is moving tests "into unit scope," yet this file imports
@test/setup-functional-testsand uses database seeders, which are characteristics of functional/integration tests. Please confirm whether:
- This is intentional (co-locating repository tests with source code while keeping functional test setup), or
- The import should be updated to a unit test setup if the goal is true unit testing with mocked dependencies.
apps/api/src/certificate/services/certificate/certificate.service.spec.ts (1)
83-123: Setup helper keeps the tests clean.Inline
setupbuilds fresh mocks and the SUT per spec, so each test stays isolated and easy to tweak.
apps/api/src/transaction/repositories/transaction/transaction.repository.spec.ts
Show resolved
Hide resolved
stalniy
force-pushed
the
refactor/functional-tests
branch
2 times, most recently
from
59d6bdd to
0d92958
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2163 +/- ##
==========================================
- Coverage 46.81% 46.41% -0.41%
==========================================
Files 1015 1006 -9
Lines 28800 28457 -343
Branches 7492 7434 -58
==========================================
- Hits 13483 13207 -276
+ Misses 14135 14068 -67
Partials 1182 1182
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
ygrishajev
previously approved these changes
Nov 6, 2025
apps/api/src/certificate/services/certificate/certificate.service.spec.ts
Outdated
Show resolved
Hide resolved
stalniy
force-pushed
the
refactor/functional-tests
branch
from
c4185c5 to
aba1781
Compare
stalniy
added a commit
that referenced
this pull request
Nov 20, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
We should use functional tests only when it makes sense, if db level or unit level test can cover functionality then we should prefer it instead of writing functional or e2e test.
e2e tests should be written only for business critical functionality
Summary by CodeRabbit
Bug Fixes
New Features
Tests