[INS 332] Create tests for stars popularity#144
Conversation
WalkthroughThis pull request introduces a new test suite for the stars data source along with a corresponding mock data file. The tests, written using the Vitest framework, validate both cumulative and new stars data retrieval by simulating API responses via a mocked function. Additionally, a set of predefined mock data structures is added to support these tests. No changes were made to exported public entities. Changes
Possibly related PRs
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-332-create-tests-for-stars-popularity
branch
from
cb52638 to
65057e6
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: 3
🧹 Nitpick comments (6)
frontend/server/data/tinybird/stars-data-source.test.ts (3)
59-59: Hardcoded percentage change valueThe
percentageChangeis hardcoded to 100 rather than calculated based on the relationship between current and previous values.For more realistic testing, calculate this value:
- percentageChange: 100, + percentageChange: previousCumulativeCount === 0 + ? 100 + : ((currentCumulativeCount - previousCumulativeCount) / previousCumulativeCount) * 100,
104-104: Hardcoded percentage change valueSimilar to the first test, the percentage change is hardcoded rather than calculated.
- percentageChange: 100, + percentageChange: previousCumulativeCount === 0 + ? 100 + : ((currentCumulativeCount - previousCumulativeCount) / previousCumulativeCount) * 100,
16-27: Test setup complexity due to module mockingThe current approach of reimporting the module in each test adds complexity.
Consider extracting the import logic into a helper function to improve readability:
+ async function getStarsDataSourceWithMocks() { + // We need to import the module again after setting the mock + return await import("~~/server/data/tinybird/stars-data-source"); + } describe('Stars Data Source', () => { beforeEach(() => { mockFetchFromTinybird.mockClear(); // Here be dragons! vi.doMock is not hoisted, and thus it is executed after the original import statement. // This means that the import for tinybird.ts inside active-contributors-data-source.ts would still be used, // and thus not mocked. This means we need to import the module again after the mock is set, whenever we want to // use it. vi.doMock(import("./tinybird"), () => ({ fetchFromTinybird: mockFetchFromTinybird, })); })Then in each test, use:
const { fetchStarsActivities } = await getStarsDataSourceWithMocks();frontend/server/mocks/tinybird-stars-response.mock.ts (3)
41-129: Unrealistic timeseries data with zero activity countsAll the cumulative activity counts are set to 0, which isn't realistic and doesn't effectively test calculations.
Consider using incrementing or varying values to better simulate real data and test calculations:
data: [ { startDate: "2024-03-01", endDate: "2024-03-31", - cumulativeActivityCount: 0 + cumulativeActivityCount: 80 }, { startDate: "2024-04-01", endDate: "2024-04-30", - cumulativeActivityCount: 0 + cumulativeActivityCount: 85 }, // Continue with increasing values
131-219: Date range inconsistency between mock data and testsThe mock data uses dates from 2023-2024, but the tests use 2024-2025, which could lead to confusion.
Either update the mock data to match the date range used in tests:
data: [ { - startDate: "2023-03-01", - endDate: "2023-03-31", + startDate: "2024-03-01", + endDate: "2024-03-31", activityCount: 0 }, // Update remaining dates similarlyOr adjust the test dates to match the mock data.
Additionally, consider using non-zero activity counts for more realistic testing:
data: [ { startDate: "2023-03-01", endDate: "2023-03-31", - activityCount: 0 + activityCount: 5 }, // Update remaining counts with varied values
1-219: Consider generating mock data programmaticallyThe mock data is very repetitive, which could lead to maintenance issues if the structure needs to change.
Consider creating a helper function to generate this data more concisely:
function generateTimeseriesData( startYear: number, startMonth: number, count: number, valueGenerator: (index: number) => number, isNew: boolean = false ) { const data = []; for (let i = 0; i < count; i++) { const currentDate = new Date(startYear, startMonth + i, 1); const year = currentDate.getFullYear(); const month = currentDate.getMonth(); // Get last day of month const lastDay = new Date(year, month + 1, 0).getDate(); data.push({ startDate: `${year}-${String(month + 1).padStart(2, '0')}-01`, endDate: `${year}-${String(month + 1).padStart(2, '0')}-${lastDay}`, [isNew ? 'activityCount' : 'cumulativeActivityCount']: valueGenerator(i) }); } return data; } // Example usage: export const mockCurrentCumulativeTimeseries = { // ... meta data ... data: generateTimeseriesData(2024, 2, 13, i => 80 + i * 5), // ... statistics ... };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between f88a04d and 65057e65b33f0ea2fc4f1eb30bf8570f63853085.
📒 Files selected for processing (2)
frontend/server/data/tinybird/stars-data-source.test.ts(1 hunks)frontend/server/mocks/tinybird-stars-response.mock.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
frontend/server/data/tinybird/stars-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.
🔇 Additional comments (2)
frontend/server/data/tinybird/stars-data-source.test.ts (1)
1-13: LGTM! Good use of imports and mocksThe imports are well organized, and the test is using appropriate mocking techniques.
frontend/server/mocks/tinybird-stars-response.mock.ts (1)
1-19: Mock data is well structured and matches API response formatThe mock summary data follows a realistic structure that matches what would be expected from the API.
There was a problem hiding this comment.
Inconsistency between test purpose and activity type
Same issue as in the first test - using FORKS instead of STARS for the activity type.
- activityType: ActivityFilterActivityType.FORKS,
+ activityType: ActivityFilterActivityType.STARS,📝 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.
| activityType: ActivityFilterActivityType.FORKS, | |
| onlyContributions: false, | |
| activityType: ActivityFilterActivityType.STARS, | |
| onlyContributions: false, |
There was a problem hiding this comment.
I'm not sure where you saw that but the code I have does not mix those types.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find and examine the stars-data-source.ts file to see how it uses activity types
rg -p stars-data-source.ts --files-with-matches
cat $(rg -p stars-data-source.ts --files-with-matches) 2>/dev/null
# Let's also check if there are any other test files that might help us understand the pattern
rg -p "test.*FORKS.*STARS|STARS.*FORKS" --files-with-matches
# Let's check how ActivityFilterActivityType is defined
rg "enum ActivityFilterActivityType" -A 10
Length of output: 840
🏁 Script executed:
#!/bin/bash
# Search for the stars-data-source.ts file using fd
fd stars-data-source.ts
# Try to search for all imports of fetchStarsActivities to understand how it's used
rg "fetchStarsActivities"
# Check if there are other data source files that might follow a similar pattern
fd -e ts -g "*data-source*"
Length of output: 2151
🏁 Script executed:
#!/bin/bash
# Let's examine the stars-data-source.ts implementation
cat frontend/server/data/tinybird/stars-data-source.ts
# Also check the forks-data-source.ts implementation for comparison
cat frontend/server/data/tinybird/forks-data-source.ts
Length of output: 7591
While the naming convention (Stars/Forks in the function names) initially led me to expect a match with the activity type parameter, I see that the implementation allows for flexibility in what activity type is passed. The primary difference between the two functions is in the property names of the returned data objects (stars vs forks).
Thank you for the correction, and I apologize for the misunderstanding.
🧠 Learnings used
Learnt from: borfast
PR: LF-Engineering/insights#144
File: frontend/server/data/tinybird/stars-data-source.test.ts:97-99
Timestamp: 2025-03-27T17:00:26.654Z
Learning: In test files for the Insights project, hardcoded values like "percentageChange: 100" are often intentional expected values for assertions, not implementation bugs. These values represent the expected calculations based on the mock data provided.
There was a problem hiding this comment.
Inconsistency between test purpose and activity type
The test is for stars data, but activityType is set to ActivityFilterActivityType.FORKS. This seems inconsistent with the test's purpose and might not be testing the correct scenario.
Consider using a stars-related activity type:
- activityType: ActivityFilterActivityType.FORKS,
+ activityType: ActivityFilterActivityType.STARS,📝 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.
| activityType: ActivityFilterActivityType.FORKS, | |
| onlyContributions: false, | |
| activityType: ActivityFilterActivityType.STARS, | |
| onlyContributions: false, |
Signed-off-by: Raúl Santos <4837+borfast@users.noreply.github.com>
borfast
force-pushed
the
improvement/ins-332-create-tests-for-stars-popularity
branch
from
65057e6 to
191e782
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
frontend/server/data/tinybird/stars-data-source.test.ts (1)
16-27: Consider extracting mock setup to a shared setup functionThe current setup with
vi.doMockand the detailed explanation could be extracted into a shared setup function or utility to reduce duplication and improve readability across test files.// Example utility function to be placed in a shared test utilities file export function setupTinybirdMock(mockImplementation: any) { // Here be dragons! vi.doMock is not hoisted, and thus it is executed after the original import statement. // This means that the import for tinybird.ts inside active-contributors-data-source.ts would still be used, // and thus not mocked. This means we need to import the module again after the mock is set, whenever we want to // use it. vi.doMock(import("./tinybird"), () => ({ fetchFromTinybird: mockImplementation, })); }frontend/server/mocks/tinybird-stars-response.mock.ts (4)
56-122: Use more realistic data for cumulative time seriesThe
mockCurrentCumulativeTimeseriesdata currently has allcumulativeActivityCountvalues set to 0, which may not adequately test scenarios where there are actual stars to count. Consider using a pattern of increasing values to simulate realistic cumulative growth.data: [ { startDate: "2024-03-01", endDate: "2024-03-31", - cumulativeActivityCount: 0 + cumulativeActivityCount: 10 }, { startDate: "2024-04-01", endDate: "2024-04-30", - cumulativeActivityCount: 0 + cumulativeActivityCount: 20 }, // ... continue with increasing values
146-212: Use more realistic data for new time seriesSimilar to the cumulative time series, the
mockCurrentNewTimeseriesdata has allactivityCountvalues set to 0. Consider using some non-zero values to test the handling of actual star activity.data: [ { startDate: "2023-03-01", endDate: "2023-03-31", - activityCount: 0 + activityCount: 5 }, { startDate: "2023-04-01", endDate: "2023-04-30", - activityCount: 0 + activityCount: 3 }, // ... continue with varied values
131-219: Ensure consistency in date ranges between mock data setsThe
mockCurrentNewTimeseriesuses date ranges from 2023-03-01 to 2024-03-31, whilemockCurrentCumulativeTimeseriesuses 2024-03-01 to 2025-03-31. This difference might lead to confusion during testing. Consider aligning the date ranges or documenting the reason for the different periods.
1-219: Consider adding TypeScript types for mock data structuresAdding explicit types for the mock data structures would improve code quality and maintainability. It would also help catch potential issues during refactoring.
interface TinybirdSummaryResponse { meta: { name: string; type: string }[]; data: { activityCount: number }[]; rows: number; statistics: { elapsed: number; rows_read: number; bytes_read: number; }; } interface TinybirdTimeseriesResponse { meta: { name: string; type: string }[]; data: { startDate: string; endDate: string; activityCount?: number; cumulativeActivityCount?: number; }[]; rows: number; statistics: { elapsed: number; rows_read: number; bytes_read: number; }; } export const mockCurrentSummaryData: TinybirdSummaryResponse = { // ...existing data };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 65057e65b33f0ea2fc4f1eb30bf8570f63853085 and 191e782.
📒 Files selected for processing (2)
frontend/server/data/tinybird/stars-data-source.test.ts(1 hunks)frontend/server/mocks/tinybird-stars-response.mock.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
frontend/server/data/tinybird/stars-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/stars-data-source.test.ts (1)
frontend/server/mocks/tinybird-stars-response.mock.ts (4)
mockCurrentSummaryData(1-19)mockPreviousSummaryData(21-39)mockCurrentCumulativeTimeseries(41-129)mockCurrentNewTimeseries(131-219)
🔇 Additional comments (2)
frontend/server/data/tinybird/stars-data-source.test.ts (2)
44-44: Inconsistency between test purpose and activity typeThe test is for stars data, but
activityTypeis set toActivityFilterActivityType.FORKS. This is inconsistent with the test's purpose and might not be testing the correct scenario.- activityType: ActivityFilterActivityType.FORKS, + activityType: ActivityFilterActivityType.STARS,
89-89: Inconsistency between test purpose and activity typeSame issue as in the first test - using
FORKSinstead ofSTARSfor the activity type.- activityType: ActivityFilterActivityType.FORKS, + activityType: ActivityFilterActivityType.STARS,
borfast
deleted the
improvement/ins-332-create-tests-for-stars-popularity
branch
2 participants
This fixes the tests for the Stars 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
Tests
Chores