Fix Issue: #1076 Date test fixed for Contribute to handle various cases.#1077
Fix Issue: #1076 Date test fixed for Contribute to handle various cases.#1077arkid15r merged 5 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
WalkthroughThe pull request introduces the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🔇 Additional comments (4)
✨ 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
Documentation and Community
|
![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 (1)
frontend/__tests__/e2e/pages/Contribute.spec.ts (1)
20-22: Great improvement to test robustness!The regex pattern now handles various time formats including "X months ago", "a month ago", "a day ago", and "X days ago", which makes the test more resilient against timing variations.
Consider adding patterns for:
- "hours ago" or "minutes ago" for very recent contributions
- "years ago" for older contributions
This would make the test even more robust for edge cases.
- page.locator('text=/\\d+ months? ago|a month ago|a day ago|\\d+ days? ago/') + page.locator('text=/\\d+ months? ago|a month ago|a day ago|\\d+ days? ago|\\d+ hours? ago|\\d+ minutes? ago|\\d+ years? ago|a year ago/')
KaranNegi20Feb
changed the title
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Yeah, this probably works. Consider this:
| test('renders issue data correctly', async ({ page }) => { | ||
| await expect(page.getByRole('link', { name: 'Contribution 1' })).toBeVisible() | ||
| await expect(page.getByText('3 months ago')).toBeVisible() | ||
| await expect( |
There was a problem hiding this comment.
This should cover all different cases: day, week, month, year -- singular and plural.
Maybe instead mock the data to always have the issue last updated date as 3 month ago?
There was a problem hiding this comment.
Alright I'll do it for day week month year👍, well as far as I have seen the data it always comes in such manner 1 day ago 15 days ago 1 month ago maybe mocking the data is the play. That's a good suggestion in respective of days if days go over the 7 (day>7 will convert it to week ). I suppose that's fine @arkid15r
There was a problem hiding this comment.
If you have 2 implementations that solve a problem always prefer the most simple one. I recommend going with the test data adjustment if that works.
There was a problem hiding this comment.
Yeah well that changing tests sounds ok to me, plus it's a small job, happy me😂
|
Hey @arkid15r i have fixed the required test cases for dates. Thank you. |
KaranNegi20Feb
changed the title
KaranNegi20Feb
changed the title
| test('renders issue data correctly', async ({ page }) => { | ||
| await expect(page.getByRole('link', { name: 'Contribution 1' })).toBeVisible() | ||
| await expect(page.getByText('3 months ago')).toBeVisible() | ||
| await expect(page.locator('text=/\\b(\\d+|a) (day|week|month|year)s? ago\\b/')).toBeVisible() |
There was a problem hiding this comment.
Why not adjust the date to be always 3 months old for this test instead?
There was a problem hiding this comment.
What exactly do we mean by that, do I need to change the mock data for testing?
There was a problem hiding this comment.
So do we both regex code and the mock data changed or just the mock data changes??
There was a problem hiding this comment.
One solution is enough and from my perspective the mock data related one is cheaper.
There was a problem hiding this comment.
Thanks, I'll make a PR in some time.
KaranNegi20Feb
force-pushed
the
date-test-fix
branch
from
76119ae to
bca87d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/__tests__/unit/data/mockContributeData.ts (2)
10-10: Consider updating theupdated_attimestamp too.While you've updated the
created_atfield to use a dynamic timestamp, theupdated_atfield on line 18 still uses a hardcoded value (1734727031). This might lead to inconsistencies in tests if they rely on both timestamps being relative to each other or to the current date.created_at: threeMonthsAgo.unix(), hint: 'Hint', labels: [], project_name: 'Owasp Nest', project_url: 'https://owasp.org/www-project-nest', repository_languages: ['Python', 'TypeScript'], summary: 'This is a summary of Contribution 1', title: 'Contribution 1', - updated_at: 1734727031, + updated_at: dayjs().subtract(2, 'months').unix(),
1-4: Consider adding a brief comment explaining the date handling approach.Adding a brief comment explaining the purpose of using dayjs for calculating relative timestamps would help other developers understand the reasoning behind this implementation, especially since it's addressing a specific issue with flaky tests.
import dayjs from 'dayjs' import relativeTime from 'dayjs/plugin/relativeTime.js' dayjs.extend(relativeTime) +// Using dynamic timestamps to prevent flaky tests caused by hardcoded dates +// This ensures tests will pass regardless of when they're run const threeMonthsAgo = dayjs().subtract(3, 'months')
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/data/mockContributeData.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
🔇 Additional comments (1)
frontend/__tests__/unit/data/mockContributeData.ts (1)
1-4: Good implementation of dynamic date calculation.Nice work implementing the dayjs library to create a dynamic "3 months ago" timestamp. This change addresses the issue of flaky tests by ensuring the test data's date always represents a relative point in time rather than a fixed timestamp.
|
Hey @arkid15r i have made sure that it always sends the date as 3 months in the relative format. |
|
…s cases. (OWASP#1077) * date test fixed * more cases added * date mock data fixed to always be 3 months * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
2 participants
Solves issue: Take care of flaky e2e tests #1076
As discussed in the issue comments the final solution can be looked here:
#1076 (comment)
#1076 (comment)
Proposed solution handles:
Used regex and it works fine passes everything.
handles for:
"3 months ago"
"a month ago"
"4 months ago"
"a day ago"
"15 days ago"
"30 days ago"