Feat/e2e testing for article sorting #1159
Conversation
|
@John-Paul-Larkin is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request involve updates to the Changes
Suggested reviewers
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 (
|
John-Paul-Larkin
temporarily deployed
to
production
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
app/(app)/articles/_client.tsx (2)
144-144: LGTM:likesprop added to ArticlePreview.The addition of the
likesprop to theArticlePreviewcomponent is consistent with the earlier changes and allows for the display of the like count. This change aligns well with the PR objectives.For improved readability, consider grouping related props together. You could move
likesnext to other user interaction-related props likebookmarkedInitialState. Here's a suggested reordering:<ArticlePreview key={id} id={id} slug={slug} title={title} excerpt={excerpt} name={user?.name || ""} username={user?.username || ""} image={user?.image || ""} date={published} readTime={readTimeMins} - bookmarkedInitialState={currentUserBookmarkedPost} likes={likes} + bookmarkedInitialState={currentUserBookmarkedPost} />
Line range hint
1-190: Overall assessment: Changes look good with a minor suggestion for future improvement.The modifications to add the
likesproperty to theArticlesPagecomponent are well-implemented and consistent with the PR objectives. The changes enhance the functionality of theArticlePreviewcomponent by allowing it to display the like count for each article.For future improvements, consider implementing pagination or infinite scrolling with a "Load More" button instead of automatically loading more articles when the user scrolls to the bottom. This approach can provide better control over data loading and potentially improve performance, especially for users with slower internet connections.
Here's a high-level suggestion for implementing a "Load More" button:
- Add a state variable to track if more posts are being loaded.
- Replace the
useInViewhook with a button click handler.- Add a button at the bottom of the article list that triggers the
fetchNextPagefunction.This change would make the data loading more explicit and give users more control over when they want to load additional content.
components/ArticlePreview/ArticlePreview.tsx (2)
43-43: LGTM: likes property added to Props type.The
likesproperty is correctly added to the Props type. However, consider making it explicitly optional for better clarity:likes?: number;This change would make it clear that the
likesproperty is not required, improving the component's API documentation.
130-138: LGTM: Likes count display implemented correctly.The likes count and heart icon are appropriately added to the component's render output. The conditional rendering and data attribute are well-implemented.
Consider adding an
aria-labelto the likes count span for better accessibility:<span aria-label={`${likes} likes`} data-likes={likes}>{likes}</span>This change would improve the experience for users relying on screen readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- app/(app)/articles/_client.tsx (2 hunks)
- components/ArticlePreview/ArticlePreview.tsx (4 hunks)
- e2e/articles.spec.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
app/(app)/articles/_client.tsx (1)
127-127: LGTM: Addition oflikesproperty.The inclusion of the
likesproperty in the destructuring of thepostsarray is consistent with the PR objectives and the AI-generated summary. This change supports the new like count feature for articles.To ensure consistency across the codebase, let's verify that the
likesproperty is properly handled in related components and API calls:components/ArticlePreview/ArticlePreview.tsx (3)
10-10: LGTM: HeartIcon import added correctly.The
HeartIconimport is appropriately added to support the new likes display feature.
59-59: LGTM: likes property correctly destructured.The
likesproperty is appropriately added to the destructured props, allowing the component to access this value.
Line range hint
1-238: Overall assessment: Well-implemented likes count feature.The changes to the
ArticlePreviewcomponent successfully implement the likes count feature as described in the PR objectives. The code is well-structured, maintains consistency with the existing codebase, and includes appropriate conditional rendering. The minor suggestions provided (explicit optional typing forlikesprop and adding anaria-labelfor accessibility) would further enhance the implementation.
| @@ -124,6 +127,15 @@ const ArticlePreview: NextPage<Props> = ({ | |||
| <> | |||
| <span aria-hidden="true">·</span> | |||
| <span>{readTime} min read</span> | |||
| {likes && ( | |||
There was a problem hiding this comment.
Oh interesting because you need a way to know how many likes an article has without clicking into it. That makes sense
There was a problem hiding this comment.
Why do we need to know the amount of likes before it's loaded? 🤔 I don't think it's shown on the card?
There was a problem hiding this comment.
Why do we need to know the amount of likes before it's loaded? 🤔 I don't think it's shown on the card?
I added it to the card in this PR.
if you take a look at the PR description you will see how it looks.
Heart icon with the like count. If there are no likes it wont show.
I figured you wouldnt like this 🤣 so I added it as a data attribute and can edit to remove it from the card.
The like count has to be included somewhere in the html, so playwright can grab it. For the sort by top test.
John-Paul-Larkin
had a problem deploying
to
production
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- e2e/articles.spec.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
e2e/articles.spec.ts (1)
224-239: Test 'Should sort articles by Newest' looks goodThe test correctly verifies that articles are sorted from newest to oldest based on their dates.
John-Paul-Larkin
had a problem deploying
to
production
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
e2e/articles.spec.ts (2)
224-239: LGTM! Consider adding a check for the "Newest" filter.The test logic for checking the newest sorting is correct and handles potential undefined dates well. However, to ensure we're testing the correct filter, consider adding a step to select or verify the "Newest" sorting option.
You could add a step to verify the current filter:
await expect(page.getByRole('button', { name: 'Newest' })).toHaveAttribute('aria-pressed', 'true');
257-276: LGTM! Consider adding error handling for missing like counts.The test logic for checking the top sorting by likes is correct. It properly navigates to the page with the "top" filter and uses the correct comparison operator (
>=) for descending order. TheparseIntfunction now includes the radix parameter as suggested in a previous comment.Consider adding error handling for cases where the likes count might be missing:
likes: parseInt( article.querySelector("[data-likes]")?.getAttribute("data-likes") ?? "0", 10 ),This ensures that if the
data-likesattribute is missing, it defaults to "0" instead of potentially passingundefinedtoparseInt.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- e2e/articles.spec.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
e2e/articles.spec.ts (2)
241-255: LGTM! Correct implementation for "Oldest" sorting test.The test logic for checking the oldest sorting is correct. It properly navigates to the page with the "oldest" filter, handles potential undefined dates, and uses the correct comparison operator (
<=) for ascending order.
224-276: Great addition of sorting tests!The new tests for sorting articles by Newest, Oldest, and Top (likes) are well-implemented and significantly improve the test coverage for the sorting functionality. They consistently handle edge cases like undefined values and use appropriate comparison logic for each sorting type.
A few minor suggestions have been made to further enhance these tests:
- Verifying the active filter in the "Newest" test.
- Adding error handling for missing like counts in the "Top" test.
Overall, these additions provide robust end-to-end testing for the article sorting feature.
John-Paul-Larkin
temporarily deployed
to
production
GitHub Actions
Inactive
|
@JohnAllenTech, this one good to go? |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- components/ArticlePreview/ArticlePreview.tsx (4 hunks)
- e2e/articles.spec.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/ArticlePreview/ArticlePreview.tsx
🧰 Additional context used
🔇 Additional comments (3)
e2e/articles.spec.ts (3)
224-239: LGTM: Newest articles sorting test implemented correctlyThe test for sorting articles by newest is well-implemented. It correctly extracts dates, handles edge cases, and verifies the descending order.
241-255: LGTM: Oldest articles sorting test implemented correctlyThe test for sorting articles by oldest is well-implemented. It correctly navigates to the filtered page, extracts dates, handles edge cases, and verifies the ascending order.
257-276: LGTM: Top articles sorting test implemented correctlyThe test for sorting articles by top (likes) is well-implemented. It correctly navigates to the filtered page, extracts likes count, handles edge cases, and verifies the descending order. The
parseIntfunction now includes the radix parameter as suggested in a previous comment.
John-Paul-Larkin
temporarily deployed
to
production
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
e2e/articles.spec.ts (4)
80-95: LGTM! Consider adding a check for the number of articles.The test logic for sorting by newest is correct and handles potential undefined dates well. To enhance the test's robustness, consider adding an assertion to check if there are actually articles present before performing the sorting check.
You could add this assertion after line 88:
expect(articles.length).toBeGreaterThan(0);
97-111: LGTM! Consider refactoring to reduce duplication.The test logic for sorting by oldest is correct and handles potential undefined dates well. However, there's significant duplication between this test and the "Newest" test.
Consider creating a helper function to reduce duplication:
function testArticleSorting(sortType: string, url: string, compareFn: (a: Date, b: Date) => boolean) { test(`Should sort articles by ${sortType}`, async ({ page }) => { await page.goto(url); await page.waitForSelector("article"); const articles = await page.$$eval("article", (articles) => { return articles.map((article) => ({ date: article.querySelector("time")?.dateTime, })); }); expect(articles.length).toBeGreaterThan(0); const isSorted = articles.every((article, index, arr) => { if (index === arr.length - 1) return true; if (!article.date || !arr[index + 1].date) return false; return compareFn(new Date(article.date), new Date(arr[index + 1].date!)); }); expect(isSorted).toBeTruthy(); }); } // Usage testArticleSorting("Newest", "http://localhost:3000/articles", (a, b) => a >= b); testArticleSorting("Oldest", "http://localhost:3000/articles?filter=oldest", (a, b) => a <= b);This refactoring would make the tests more maintainable and easier to extend in the future.
113-132: LGTM! Consider refactoring and adding error handling.The test logic for sorting by top likes is correct. However, there are a few improvements that can be made:
- Error handling for parsing likes count.
- Reducing duplication with other sorting tests.
- Adding a check for the number of articles.
Consider updating the helper function suggested earlier to include this test:
function testArticleSorting<T>( sortType: string, url: string, extractFn: (article: Element) => T, compareFn: (a: T, b: T) => boolean ) { test(`Should sort articles by ${sortType}`, async ({ page }) => { await page.goto(url); await page.waitForSelector("article"); const articles = await page.$$eval("article", (articles, extract) => articles.map(extract as (article: Element) => T), extractFn); expect(articles.length).toBeGreaterThan(0); const isSorted = articles.every((article, index, arr) => index === arr.length - 1 || compareFn(article, arr[index + 1])); expect(isSorted).toBeTruthy(); }); } // Usage testArticleSorting( "Top - likes", "http://localhost:3000/articles?filter=top", (article) => { const likesStr = article.querySelector("[data-likes]")?.getAttribute("data-likes") || "0"; const likes = parseInt(likesStr, 10); if (isNaN(likes)) { throw new Error(`Invalid likes count: ${likesStr}`); } return likes; }, (a, b) => a >= b );This refactoring addresses the duplication issue and adds error handling for parsing the likes count.
80-132: Overall, good addition of sorting tests. Consider refactoring for improved maintainability.The new tests for sorting articles by Newest, Oldest, and Top likes are valuable additions to the test suite. They cover important functionality and are generally well-implemented. However, there are a few areas for improvement:
- Reduce code duplication by creating a helper function for sorting tests.
- Add checks for the number of articles before performing sorting checks.
- Improve error handling, particularly for parsing likes count.
- Consider using Playwright's
baseURLconfiguration to avoid hardcoding URLs in tests.Implementing these suggestions will make the tests more robust and easier to maintain in the future.
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
✨ Codu Pull Request 💻
Pull Request details
Add Playwright tests for sorting on the article page
Run test for three sorting modes - "Newest", "Oldest" and "Top"
Like count added to ArticlePreview
I was required to pass the like count to the component, so If it non-zero, I display it on the Article preview with the heart icon.
I also added the count as a data attribute, so we can easily remove this if you dont like how it looks.
Any Breaking changes