[INS 333] Create tests for forks popularity#143
Conversation
WalkthroughThis pull request adds unit tests and mock data for Tinybird forks functionality. The new test file verifies the Changes
Suggested Reviewers
✨ Finishing Touches
🪧 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 (
|
borfast
force-pushed
the
improvement/add-github-action-for-tests
branch
from
8325e68 to
6c99d03
Compare
borfast
force-pushed
the
improvement/ins-333-create-tests-for-forks-popularity
branch
from
22ce413 to
4e38606
Compare
Signed-off-by: Raúl Santos <4837+borfast@users.noreply.github.com>
borfast
force-pushed
the
improvement/ins-333-create-tests-for-forks-popularity
branch
from
4e38606 to
65976f8
Compare
borfast
deleted the
improvement/ins-333-create-tests-for-forks-popularity
branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/server/data/tinybird/forks-data-source.test.ts (3)
29-72: Add verification of parameters passed to the mock functionWhile the test correctly verifies the output structure, it doesn't verify that
fetchFromTinybirdwas called with the correct parameters for each request.Add expectations to verify the correct parameters were passed:
expect(result).toEqual(expectedResult); + + // Verify that fetchFromTinybird was called with correct parameters + expect(mockFetchFromTinybird).toHaveBeenCalledTimes(3); + expect(mockFetchFromTinybird.mock.calls[0][1]).toMatchObject({ + project: 'the-linux-kernel-organization', + activityType: ActivityFilterActivityType.FORKS, + });
55-63: Calculate percentageChange dynamically instead of hardcodingThe percentageChange is hardcoded to 100, which may not accurately reflect the actual percentage change between the current and previous values.
summary: { current: currentCumulativeCount, previous: previousCumulativeCount, - percentageChange: 100, + percentageChange: previousCumulativeCount === 0 + ? 0 + : ((currentCumulativeCount - previousCumulativeCount) / previousCumulativeCount) * 100, changeValue: currentCumulativeCount - previousCumulativeCount, periodFrom: filter.startDate?.toString(), periodTo: filter.endDate?.toString(), },
74-117: Similar issue with hardcoded percentageChange in the second testLike in the first test, the percentageChange is hardcoded to 100 instead of being calculated based on the actual values.
summary: { current: currentCumulativeCount, previous: previousCumulativeCount, - percentageChange: 100, + percentageChange: previousCumulativeCount === 0 + ? 0 + : ((currentCumulativeCount - previousCumulativeCount) / previousCumulativeCount) * 100, changeValue: currentCumulativeCount - previousCumulativeCount, periodFrom: filter.startDate?.toString(), periodTo: filter.endDate?.toString(), },frontend/server/mocks/tinybird-forks-response.mock.ts (3)
41-129: Consider adding non-zero values to the cumulative timeseries dataAll cumulative activity counts are set to 0, which might not test real-world scenarios effectively. For a better test, consider adding increasing values over time, which is expected behavior for cumulative metrics.
{ startDate: "2024-03-01", endDate: "2024-03-31", - cumulativeActivityCount: 0 + cumulativeActivityCount: 10 }, { startDate: "2024-04-01", endDate: "2024-04-30", - cumulativeActivityCount: 0 + cumulativeActivityCount: 25 }, { startDate: "2024-05-01", endDate: "2024-05-31", - cumulativeActivityCount: 0 + cumulativeActivityCount: 40 },
131-219: Add some non-zero values to new timeseries dataLike the cumulative data, the new activity counts are all set to 0. For more effective testing, consider adding varying (non-zero) values to ensure the visualization can handle peaks and trends.
{ startDate: "2023-03-01", endDate: "2023-03-31", - activityCount: 0 + activityCount: 5 }, { startDate: "2023-04-01", endDate: "2023-04-30", - activityCount: 0 + activityCount: 12 }, { startDate: "2023-05-01", endDate: "2023-05-31", - activityCount: 0 + activityCount: 8 },
1-219: Consider creating a mock data factory functionThe mock objects share similar structures but with different field names and values. Consider creating a factory function to generate these mock objects to reduce duplication and make it easier to create varied test cases.
function createTimeseriesMock(options: { field: string; startYear: number; startMonth: number; count: number; values?: number[]; }) { const { field, startYear, startMonth, count, values = [] } = options; const data = []; for (let i = 0; i < count; i++) { const currentMonth = ((startMonth - 1 + i) % 12) + 1; const yearOffset = Math.floor((startMonth - 1 + i) / 12); const year = startYear + yearOffset; // Calculate days in month const daysInMonth = new Date(year, currentMonth, 0).getDate(); data.push({ startDate: `${year}-${String(currentMonth).padStart(2, '0')}-01`, endDate: `${year}-${String(currentMonth).padStart(2, '0')}-${daysInMonth}`, [field]: values[i] || 0 }); } return { meta: [ { name: "startDate", type: "Nullable(Date)" }, { name: "endDate", type: "Nullable(Date)" }, { name: field, type: field.includes("cumulative") ? "Nullable(UInt64)" : "UInt64" } ], data, rows: data.length, statistics: { elapsed: 0.123456789, rows_read: 100000, bytes_read: 5000000 } }; } // Example usage: export const mockCurrentCumulativeTimeseries = createTimeseriesMock({ field: "cumulativeActivityCount", startYear: 2024, startMonth: 3, count: 13, values: [10, 25, 40, 60, 75, 90, 105, 120, 135, 150, 165, 180, 200] });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/server/data/tinybird/forks-data-source.test.ts(1 hunks)frontend/server/mocks/tinybird-forks-response.mock.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
frontend/server/data/tinybird/forks-data-source.test.ts (1)
Learnt from: borfast
PR: LF-Engineering/insights#76
File: frontend/server/data/tinybird/active-contributors-data-source.test.ts:10-10
Timestamp: 2025-03-27T11:19:30.505Z
Learning: The tests in the frontend/server/data/tinybird directory became broken due to changes introduced in another branch and will need to be revisited later as part of the data sources refactoring.
🧬 Code Definitions (1)
frontend/server/data/tinybird/forks-data-source.test.ts (1)
frontend/server/mocks/tinybird-forks-response.mock.ts (4)
mockCurrentSummaryData(1-19)mockPreviousSummaryData(21-39)mockCurrentCumulativeTimeseries(41-129)mockCurrentNewTimeseries(131-219)
🔇 Additional comments (4)
frontend/server/data/tinybird/forks-data-source.test.ts (3)
1-13: Import structure looks goodThe imports are well-organized and include all necessary dependencies for testing the forks data source. Using named imports helps with code clarity.
16-27: Good explanation of vi.doMock behaviorThe detailed comment explaining the non-hoisted behavior of vi.doMock is valuable for other developers, as this is a common source of confusion in Vitest. The mock setup correctly addresses this limitation.
119-120:Details
✅ Verification successful
Good TO-DO comment for future test improvements
Having a clear TODO comment about handling edge cases is good practice. Consider creating a ticket to track this work to ensure these tests get implemented.
To help plan these future tests, here's a quick verification of what edge cases might be relevant:
🏁 Script executed:
#!/bin/bash # Description: Look for existing edge case tests in other data source test files for patterns to follow # Search for patterns related to error handling in existing test files echo "Searching for existing error handling patterns in data source tests..." rg -A 3 "invalid|error|throw|catch|reject|fail" --type ts --glob "*-data-source.test.ts" || echo "No matching patterns found"Length of output: 1434
Edge-Case Test Improvements – Approved with Ticket Recommendation
The TODO comment in
frontend/server/data/tinybird/forks-data-source.test.ts(lines 119–120) clearly outlines the need for additional checks (invalid dates, invalid data, SQL injections, and other edge cases). This pattern is consistent with similar TODOs in other data source test files. It's a good marker for future improvements. Please consider creating a tracking ticket to ensure this work is prioritized and implemented uniformly.frontend/server/mocks/tinybird-forks-response.mock.ts (1)
1-39: Mock summary data structure looks goodThe summary mock data for current and previous periods follows a consistent structure that matches the expected Tinybird response format. The difference between the two values (100 vs 50) provides a good test case for calculating changes.
2 participants
This fixes the tests for the �Forks data source.
It just takes care of the most basic test. The goal is simply to have a basic security harness so we can refactor things without fear of breaking stuff.
Summary by CodeRabbit