Testes para a função updateTrending #560
Closed
Conversation
This file contains 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
|
@WillxBernardo is attempting to deploy a commit to the Listinai Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces configuration updates for Jest testing, enhancing the project's TypeScript testing capabilities. The changes include modifying the Jest configuration to support TypeScript, updating the package.json to include a test script, adding a new test suite for an Changes
Sequence DiagramsequenceDiagram
participant Service as YourService
participant StarsRepo as Stars Repository
participant NotifyService as Notification Service
Service->>StarsRepo: Retrieve current trending items
Service->>Service: Compare with new trending list
alt Changes detected
Service->>Service: Identify removed/changed items
Service->>NotifyService: Trigger notifications
end
Poem
Finishing Touches
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
jest.config.ts (1)
3-10: Consider enhancing the Jest configuration for better test coverage and specificity.
- Add coverage configuration to track test coverage metrics.
- Make test patterns more specific to avoid unintended matches.
module.exports = { preset: 'ts-jest', // Usando ts-jest para suportar TypeScript testEnvironment: 'node', // Ambiente de teste para Node.js moduleFileExtensions: ['js', 'json', 'ts'], testMatch: ['**/tests/**/*.spec.ts', '**/tests/**/*.test.ts'], // Caminho para os testes testPathIgnorePatterns: ['/node_modules/'], // Ignora a pasta node_modules verbose: true, // Exibe detalhes adicionais ao executar os testes + collectCoverage: true, + coverageDirectory: 'coverage', + coverageReporters: ['text', 'lcov'], + testMatch: [ + '**/tests/**/__tests__/**/*.[jt]s?(x)', + '**/tests/**/?(*.)+(spec|test).[jt]s?(x)' + ], };tests/updateTrending.test.ts (1)
4-7: Add type definitions for mocked dependencies.The mocked dependencies should have proper TypeScript interfaces/types for better type safety.
+import { NotificationService } from '@gitroom/nestjs-libraries/database/prisma/notifications/notification.service'; +import { StarsRepository } from '@gitroom/nestjs-libraries/database/prisma/stars/stars.repository'; + // Mock das dependências jest.mock('@gitroom/nestjs-libraries/database/prisma/stars/stars.repository'); jest.mock('@gitroom/nestjs-libraries/database/prisma/notifications/notification.service'); jest.mock('@gitroom/nestjs-libraries/bull-mq-transport-new/client');tsconfig.base.json (1)
34-39: Consider more specific patterns for test files.The current patterns might include unnecessary files. Consider organizing tests in a more structured way.
"include": [ - "tests/**/*.spec.ts", - "tests/**/*.test.ts", + "tests/**/__tests__/**/*.ts", + "tests/**/*.spec.ts", + "tests/**/*.test.ts", "libs/**/*.ts", "apps/**/*.ts" ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
jest.config.ts(1 hunks)package.json(2 hunks)tests/updateTrending.test.ts(1 hunks)tsconfig.base.json(1 hunks)
🔇 Additional comments (1)
package.json (1)
32-33: Verify Jest version compatibility.The caret version for Jest (
^29.7.0) should be verified against other testing-related dependencies to ensure compatibility.✅ Verification successful
Jest ecosystem dependencies are properly aligned ✅
All Jest-related packages are using compatible versions within the 29.x range.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check versions of related testing packages echo "Checking versions of testing-related packages:" npm list jest jest-environment-jsdom jest-environment-node ts-jest @types/jestLength of output: 438
Script:
#!/bin/bash # Check if ts-jest or @types/jest are listed in package.json cat package.json | jq '.dependencies, .devDependencies'Length of output: 5438
| return current && current.position !== p.position; | ||
| }); | ||
| if (removedFromTrending.length) { | ||
| await this.notificationService.inform(Inform.Removed, removedFromTrending, language); |
There was a problem hiding this comment.
Define the Inform enum for better code clarity.
The Inform enum is used but not defined, which could lead to maintenance issues.
+enum Inform {
+ Removed = 1,
+ Changed = 2,
+ New = 3
+}
+
// ... rest of the codeAlso applies to: 60-60, 68-68
Comment on lines
+88
to
+96
| test('CT1: Teste com data vazio', async () => { | ||
| const data: Array<{ name: string; position: number }> = []; | ||
|
|
||
| await service.updateTrending('en', 'hash1', data); | ||
|
|
||
| expect(mockStarsRepository.getTrendingByLanguage).not.toHaveBeenCalled(); | ||
| expect(mockNotificationService.inform).not.toHaveBeenCalled(); | ||
| expect(mockReplaceOrAddTrending).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance empty data test case.
The test case for empty data should verify that the function handles the case gracefully, but it's currently not testing the actual behavior.
- test('CT1: Teste com data vazio', async () => {
+ test('should not process updates when trending list is empty', async () => {
const data: Array<{ name: string; position: number }> = [];
await service.updateTrending('en', 'hash1', data);
- expect(mockStarsRepository.getTrendingByLanguage).not.toHaveBeenCalled();
+ expect(mockStarsRepository.getTrendingByLanguage).toHaveBeenCalledWith('en');
expect(mockNotificationService.inform).not.toHaveBeenCalled();
expect(mockReplaceOrAddTrending).not.toHaveBeenCalled();
});📝 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.
Suggested change
| test('CT1: Teste com data vazio', async () => { | |
| const data: Array<{ name: string; position: number }> = []; | |
| await service.updateTrending('en', 'hash1', data); | |
| expect(mockStarsRepository.getTrendingByLanguage).not.toHaveBeenCalled(); | |
| expect(mockNotificationService.inform).not.toHaveBeenCalled(); | |
| expect(mockReplaceOrAddTrending).not.toHaveBeenCalled(); | |
| }); | |
| test('should not process updates when trending list is empty', async () => { | |
| const data: Array<{ name: string; position: number }> = []; | |
| await service.updateTrending('en', 'hash1', data); | |
| expect(mockStarsRepository.getTrendingByLanguage).toHaveBeenCalledWith('en'); | |
| expect(mockNotificationService.inform).not.toHaveBeenCalled(); | |
| expect(mockReplaceOrAddTrending).not.toHaveBeenCalled(); | |
| }); |
Comment on lines
+19
to
+32
| class YourService { | ||
| private _starsRepository; | ||
| private notificationService; | ||
| private replaceOrAddTrending; | ||
|
|
||
| constructor( | ||
| _starsRepository: any, | ||
| notificationService: any, | ||
| replaceOrAddTrending: any | ||
| ) { | ||
| this._starsRepository = _starsRepository; | ||
| this.notificationService = notificationService; | ||
| this.replaceOrAddTrending = replaceOrAddTrending; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety in the service class.
The service class uses any types which should be avoided for better type safety.
+interface TrendingItem {
+ name: string;
+ position: number;
+}
+
class YourService {
- private _starsRepository;
- private notificationService;
- private replaceOrAddTrending;
+ private _starsRepository: StarsRepository;
+ private notificationService: NotificationService;
+ private replaceOrAddTrending: (language: string, hash: string, arr: TrendingItem[]) => Promise<void>;
constructor(
- _starsRepository: any,
- notificationService: any,
- replaceOrAddTrending: any
+ _starsRepository: StarsRepository,
+ notificationService: NotificationService,
+ replaceOrAddTrending: (language: string, hash: string, arr: TrendingItem[]) => Promise<void>
)Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+34
to
+70
| async updateTrending( | ||
| language: string, | ||
| hash: string, | ||
| arr: Array<{ name: string; position: number }> | ||
| ) { | ||
| const currentTrending = await this._starsRepository.getTrendingByLanguage(language); | ||
|
|
||
| if (currentTrending?.hash === hash) { | ||
| return; | ||
| } | ||
|
|
||
| if (currentTrending) { | ||
| const list: Array<{ name: string; position: number }> = JSON.parse( | ||
| currentTrending.trendingList | ||
| ); | ||
| const removedFromTrending = list.filter( | ||
| (p) => !arr.find((a) => a.name === p.name) | ||
| ); | ||
| const changedPosition = arr.filter((p) => { | ||
| const current = list.find((a) => a.name === p.name); | ||
| return current && current.position !== p.position; | ||
| }); | ||
| if (removedFromTrending.length) { | ||
| await this.notificationService.inform(Inform.Removed, removedFromTrending, language); | ||
| } | ||
| if (changedPosition.length) { | ||
| await this.notificationService.inform(Inform.Changed, changedPosition, language); | ||
| } | ||
| } | ||
|
|
||
| const informNewPeople = arr.filter( | ||
| (p) => !currentTrending?.trendingList || currentTrending?.trendingList.indexOf(p.name) === -1 | ||
| ); | ||
|
|
||
| await this.notificationService.inform(Inform.New, informNewPeople, language); | ||
| await this.replaceOrAddTrending(language, hash, arr); | ||
| } |
There was a problem hiding this comment.
Add input validation to updateTrending method.
The method should validate its inputs before processing to prevent invalid data.
async updateTrending(
language: string,
hash: string,
arr: Array<{ name: string; position: number }>
) {
+ if (!language || typeof language !== 'string') {
+ throw new Error('Invalid language parameter');
+ }
+ if (!hash || typeof hash !== 'string') {
+ throw new Error('Invalid hash parameter');
+ }
+ if (!Array.isArray(arr)) {
+ throw new Error('Invalid array parameter');
+ }
+ for (const item of arr) {
+ if (!item.name || typeof item.name !== 'string') {
+ throw new Error('Invalid item name');
+ }
+ if (typeof item.position !== 'number' || isNaN(item.position)) {
+ throw new Error('Invalid position value');
+ }
+ }
+
const currentTrending = await this._starsRepository.getTrendingByLanguage(language);📝 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.
Suggested change
| async updateTrending( | |
| language: string, | |
| hash: string, | |
| arr: Array<{ name: string; position: number }> | |
| ) { | |
| const currentTrending = await this._starsRepository.getTrendingByLanguage(language); | |
| if (currentTrending?.hash === hash) { | |
| return; | |
| } | |
| if (currentTrending) { | |
| const list: Array<{ name: string; position: number }> = JSON.parse( | |
| currentTrending.trendingList | |
| ); | |
| const removedFromTrending = list.filter( | |
| (p) => !arr.find((a) => a.name === p.name) | |
| ); | |
| const changedPosition = arr.filter((p) => { | |
| const current = list.find((a) => a.name === p.name); | |
| return current && current.position !== p.position; | |
| }); | |
| if (removedFromTrending.length) { | |
| await this.notificationService.inform(Inform.Removed, removedFromTrending, language); | |
| } | |
| if (changedPosition.length) { | |
| await this.notificationService.inform(Inform.Changed, changedPosition, language); | |
| } | |
| } | |
| const informNewPeople = arr.filter( | |
| (p) => !currentTrending?.trendingList || currentTrending?.trendingList.indexOf(p.name) === -1 | |
| ); | |
| await this.notificationService.inform(Inform.New, informNewPeople, language); | |
| await this.replaceOrAddTrending(language, hash, arr); | |
| } | |
| async updateTrending( | |
| language: string, | |
| hash: string, | |
| arr: Array<{ name: string; position: number }> | |
| ) { | |
| if (!language || typeof language !== 'string') { | |
| throw new Error('Invalid language parameter'); | |
| } | |
| if (!hash || typeof hash !== 'string') { | |
| throw new Error('Invalid hash parameter'); | |
| } | |
| if (!Array.isArray(arr)) { | |
| throw new Error('Invalid array parameter'); | |
| } | |
| for (const item of arr) { | |
| if (!item.name || typeof item.name !== 'string') { | |
| throw new Error('Invalid item name'); | |
| } | |
| if (typeof item.position !== 'number' || isNaN(item.position)) { | |
| throw new Error('Invalid position value'); | |
| } | |
| } | |
| const currentTrending = await this._starsRepository.getTrendingByLanguage(language); | |
| if (currentTrending?.hash === hash) { | |
| return; | |
| } | |
| if (currentTrending) { | |
| const list: Array<{ name: string; position: number }> = JSON.parse( | |
| currentTrending.trendingList | |
| ); | |
| const removedFromTrending = list.filter( | |
| (p) => !arr.find((a) => a.name === p.name) | |
| ); | |
| const changedPosition = arr.filter((p) => { | |
| const current = list.find((a) => a.name === p.name); | |
| return current && current.position !== p.position; | |
| }); | |
| if (removedFromTrending.length) { | |
| await this.notificationService.inform(Inform.Removed, removedFromTrending, language); | |
| } | |
| if (changedPosition.length) { | |
| await this.notificationService.inform(Inform.Changed, changedPosition, language); | |
| } | |
| } | |
| const informNewPeople = arr.filter( | |
| (p) => !currentTrending?.trendingList || currentTrending?.trendingList.indexOf(p.name) === -1 | |
| ); | |
| await this.notificationService.inform(Inform.New, informNewPeople, language); | |
| await this.replaceOrAddTrending(language, hash, arr); | |
| } |
Comment on lines
+120
to
+136
| test('CT3: Teste com data contendo valores inválidos', async () => { | ||
| const data: Array<{ name: string; position: number }> = [ | ||
| { name: 'item1', position: Number('string') }, // Garantir que seja número | ||
| { name: 'item2', position: 3 }, | ||
| ]; | ||
|
|
||
| mockStarsRepository.getTrendingByLanguage.mockResolvedValue({ | ||
| hash: 'hash1', | ||
| trendingList: JSON.stringify([{ name: 'item3', position: 6 }]), | ||
| }); | ||
|
|
||
| await service.updateTrending('en', 'hash2', data); | ||
|
|
||
| expect(mockStarsRepository.getTrendingByLanguage).toHaveBeenCalled(); | ||
| expect(mockNotificationService.inform).not.toHaveBeenCalled(); | ||
| expect(mockReplaceOrAddTrending).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve invalid data test case.
The test case for invalid data should be more explicit about what makes the data invalid and what behavior is expected.
- test('CT3: Teste com data contendo valores inválidos', async () => {
+ test('should not process updates when position values are invalid numbers', async () => {
const data: Array<{ name: string; position: number }> = [
- { name: 'item1', position: Number('string') }, // Garantir que seja número
+ { name: 'item1', position: NaN }, // Invalid position
{ name: 'item2', position: 3 },
];
mockStarsRepository.getTrendingByLanguage.mockResolvedValue({
hash: 'hash1',
trendingList: JSON.stringify([{ name: 'item3', position: 6 }]),
});
await service.updateTrending('en', 'hash2', data);
expect(mockStarsRepository.getTrendingByLanguage).toHaveBeenCalled();
- expect(mockNotificationService.inform).not.toHaveBeenCalled();
- expect(mockReplaceOrAddTrending).not.toHaveBeenCalled();
+ expect(() => service.updateTrending('en', 'hash2', data)).rejects.toThrow('Invalid position value');
});Committable suggestion skipped: line range outside the PR's diff.
|
I am not sure what is this, are you just adding stuff with AI? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Descrição
Este pull request adiciona testes unitários para o método updateTrending e inclui configurações adicionais necessárias para garantir a execução correta dos testes no ambiente atual. O objetivo principal é aumentar a cobertura de testes e validar cenários críticos relacionados à atualização de tendências.
Principais mudanças:
Adição de Testes Unitários:
Criados mocks para as seguintes dependências ausentes:
Além da adição do Jest para criação de mocks e isolamento de dependências.
Summary by CodeRabbit
New Features
Chores
testscript to run JestTests