Conversation
WalkthroughThis pull request introduces a new test file for the Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Runner
participant F as fetchContributorDependency
participant M as fetchFromTinybird (Mock)
T->>F: Invoke fetchContributorDependency(filter)
F->>M: Call fetchFromTinybird (first endpoint)
M-->>F: Return mocked response data
F->>M: Call fetchFromTinybird (second endpoint)
M-->>F: Return mocked response data
F-->>T: Return aggregated contributor data
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/ins-328-create-tests-for-contributors-dependency
branch
from
757d8fb to
c14a12d
Compare
borfast
changed the base branch from
improvement/add-github-action-for-tests
to
improvement/ins-330-create-tests-for-contributors-leaderboard
borfast
force-pushed
the
improvement/ins-330-create-tests-for-contributors-leaderboard
branch
2 times, most recently
from
89e46f3 to
3e789b4
Compare
Signed-off-by: Raúl Santos <4837+borfast@users.noreply.github.com>
borfast
force-pushed
the
improvement/ins-328-create-tests-for-contributors-dependency
branch
from
c14a12d to
dc1964e
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: 0
🧹 Nitpick comments (4)
frontend/server/mocks/tinybird-contributors-dependency-response.mock.ts (1)
1-103: Good job creating a comprehensive mock data structure.The
mockTimeseriesstructure provides a realistic representation of Tinybird API response data, including metadata, contributor information, and performance statistics. The mock includes a variety of contributors with different contribution percentages, which will be useful for testing various scenarios.One observation: there's a slight inconsistency in the
contributionPercentageRunningTotalvalues. For example, Christian Brauner has 16% (line 78), followed by Krzysztof Kozlowski at 15% (line 85), then Bjorn Andersson at 17% (line 92). This suggests the data isn't sorted by running total. This may be intentional for testing purposes, but if the API typically returns data in a specific order, consider adjusting the mock to match that behavior.frontend/server/data/tinybird/contributors-dependency-data-source.test.ts (3)
15-18: Update the comment reference to match this test file.The comment refers to "active-contributors-data-source.ts" but should refer to "contributors-dependency-data-source.ts" since that's the module being tested in this file.
- // This means that the import for tinybird.ts inside active-contributors-data-source.ts would still be used, + // This means that the import for tinybird.ts inside contributors-dependency-data-source.ts would still be used,
80-80: Consider implementing the mentioned edge case tests.The TODO comment identifies important edge cases to test (invalid dates, invalid data, SQL injections). Since this PR aims to "fix the existing tests" and "implement the most fundamental test to establish a basic security harness," adding at least basic validation for these edge cases would strengthen the testing framework before future refactoring.
Example implementation:
// TODO: Add checks for invalid dates, invalid data, sql injections, and other edge cases. + + test('should handle invalid dates gracefully', async () => { + const { fetchContributorDependency } = await import("~~/server/data/tinybird/contributors-dependency-data-source"); + + mockFetchFromTinybird.mockResolvedValueOnce({ data: [], meta: [], statistics: {} }); + mockFetchFromTinybird.mockResolvedValueOnce({ data: [], meta: [], statistics: {} }); + + const filter = { + project: 'the-linux-kernel-organization', + startDate: DateTime.utc(2025, 3, 20), + endDate: DateTime.utc(2024, 3, 20) // End date before start date + }; + + const result = await fetchContributorDependency(filter); + + // Expect valid but empty response structure + expect(result).toEqual({ + topContributors: { count: 0, percentage: 0 }, + otherContributors: { count: 0, percentage: 100 }, + list: [] + }); + }); + + test('should sanitize project name to prevent SQL injection', async () => { + const { fetchContributorDependency } = await import("~~/server/data/tinybird/contributors-dependency-data-source"); + + mockFetchFromTinybird.mockResolvedValueOnce(mockTimeseries).mockResolvedValueOnce(mockLeaderboardTimeseries); + + const filter = { + project: "project'; DROP TABLE contributors; --", + startDate: DateTime.utc(2024, 3, 20), + endDate: DateTime.utc(2025, 3, 20) + }; + + await fetchContributorDependency(filter); + + // Check if the project name is passed directly to the API or if it's sanitized + const firstCallArgs = mockFetchFromTinybird.mock.calls[0][1]; + expect(firstCallArgs.project).toBe("project'; DROP TABLE contributors; --"); + + // This test would fail if the implementation doesn't properly handle SQL injection attempts + // Note: A more comprehensive test would check if the actual implementation sanitizes the input + });
55-59: Consider adding a check for empty data handling.The current implementation extracts values like
topContributorsPercentageandtotalContributorCountfrom the mock data without fully handling the case where that data might be empty. While the optional chaining helps prevent errors, it would be good to add a test case that explicitly verifies behavior with empty data responses.// Add a test case in the file: + test('should handle empty data responses gracefully', async () => { + const { fetchContributorDependency } = await import("~~/server/data/tinybird/contributors-dependency-data-source"); + + // Mock empty responses + mockFetchFromTinybird.mockResolvedValueOnce({ + data: [], + meta: mockTimeseries.meta, + statistics: mockTimeseries.statistics + }).mockResolvedValueOnce({ + data: [], + meta: mockLeaderboardTimeseries.meta, + statistics: mockLeaderboardTimeseries.statistics + }); + + const filter = { + project: 'the-linux-kernel-organization', + startDate: DateTime.utc(2024, 3, 20), + endDate: DateTime.utc(2025, 3, 20) + }; + + const result = await fetchContributorDependency(filter); + + // Expected result with empty data + const expectedResult: ContributorDependencyResponse = { + topContributors: { + count: 0, + percentage: 0 + }, + otherContributors: { + count: 0, + percentage: 100 + }, + list: [] + }; + + expect(result).toEqual(expectedResult); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/server/data/tinybird/contributors-dependency-data-source.test.ts(1 hunks)frontend/server/mocks/tinybird-contributors-dependency-response.mock.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
frontend/server/data/tinybird/contributors-dependency-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/contributors-dependency-data-source.test.ts (1)
frontend/server/mocks/tinybird-contributors-dependency-response.mock.ts (1)
mockTimeseries(1-103)
🔇 Additional comments (2)
frontend/server/data/tinybird/contributors-dependency-data-source.test.ts (2)
24-78: Well-structured test with good verification of parameters and response.The test effectively validates that the
fetchContributorDependencyfunction calls the Tinybird API with the correct parameters and properly formats the response. Good job calculating the expected values based on the mock data.
1-81: Great first implementation of the test structure for Contributors Dependency.This is a good start for testing the contributors dependency feature. The test covers the basic functionality and establishes a solid foundation for future tests. I see this aligns with the PR objective to "implement the most fundamental test to establish a basic security harness."
Based on the retrieved learning that "tests in the frontend/server/data/tinybird directory became broken due to changes introduced in another branch," it's important that these new tests remain robust during future refactoring efforts.
borfast
deleted the
improvement/ins-328-create-tests-for-contributors-dependency
branch
2 participants
This fixes the tests for the Contributors Dependency 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