test: add testing for isVotingWithinLastThreeMonths function #1837
Conversation
Nandgopal-R
requested review from
quetzalliwrites,
derberg,
asyncapi-bot-eve and
thulieblack
as code owners
Nandgopal-R
changed the title
There was a problem hiding this comment.
- sorry, please rename
testingtotestto follow standard naming - I don't know why I suggestedtestinginitially 😄 - you forgot to add docs (first bullet point in requirements)
- provide jsdoc for
isVotingWithinLastThreeMonthsand put there an example ofvoteInfoargument - I think that instead of
vote_tracker_utils.jswe should rather add there a new folder, as we already have some, like maintainers- .github - scripts - vote_tracker - index.js (previously `vote_tracker.js`) - utils.js
wdyt?
|
I thought of this: .github
└── scripts
├── vote_tracker
│ ├── index.js # Main vote tracking logic
├── vote_utils
│ ├── utils.js # Utility functions for vote tracking This way, the core logic and utilities are separate. What do you think? |
…d jsdoc for isVotingWithinLastThreeMonths
|
but it ain't a big project, so does it make sense to introduce such folder structure? especially that |
Yeah, fair point. I'll follow the other one and keep it simple. And by the way, I have added the jsdoc and the |
|
ping me whenever ready for another review round |
|
@Nandgopal-R any news from your side? |
WalkthroughThis update refactors the vote tracking script for TSC member voting on GitHub by moving the main logic from a single file to a new modular structure. The core vote tracking workflow is now in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Actions
participant VoteTracker (index.js)
participant Utils (utils.js)
participant Filesystem
User->>GitHub Actions: Trigger workflow (via comment or bot URL)
GitHub Actions->>VoteTracker: Invoke exported async function
VoteTracker->>GitHub Actions: Fetch comment/issue data
VoteTracker->>Filesystem: Read MAINTAINERS.yaml and voteTrackingFile.json
VoteTracker->>Utils: Call isVotingWithinLastThreeMonths
Utils-->>VoteTracker: Return recent voting status
VoteTracker->>Filesystem: Update voteTrackingFile.json
VoteTracker->>Filesystem: Write TSC_VOTING_OVERVIEW.md
VoteTracker-->>GitHub Actions: Finish workflow
Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
|
@derberg I have made the changes |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/scripts/vote_tracker/utils.js (1)
45-60: Good implementation with proper edge case handling.The function correctly:
- Handles cases where a member has no voting history
- Rejects future dates as invalid
- Calculates the difference in days accurately
One minor suggestion would be to extract the "90 days" as a named constant at the top of the file for better maintainability.
+ // Number of days considered as "within 3 months" + const DAYS_IN_THREE_MONTHS = 90; function isVotingWithinLastThreeMonths(voteInfo) { // ...existing code... const diffInDays = (currentDate - lastVoteDate) / (1000 * 60 * 60 * 24); - return diffInDays <= 90; // 90 days = 3 months + return diffInDays <= DAYS_IN_THREE_MONTHS; }package.json (1)
1-8: Basic package.json setup for testing.The package.json file correctly sets up Jest for testing but is missing standard metadata fields. Consider adding name, version, description, and other relevant fields to make the package.json more complete and informative.
{ + "name": "asyncapi-community", + "version": "1.0.0", + "description": "AsyncAPI Community repository", + "private": true, "scripts": { "test": "jest" }, "devDependencies": { "jest": "^29.0.0" } }.github/scripts/vote_tracker/index.js (2)
63-72: Avoid double lookup of the same vote row
userVoteanduserInfoare computed with identicalfindcalls. This duplicates work and complicates reasoning. Re-use the first lookup result:-const userVote = latestVotes.find(...); +const userVote = latestVotes.find( + (vote) => vote.user.toLowerCase() === voteInfo.name.toLowerCase() +); /* …later… */ -const userInfo = latestVotes.find( - (vote) => vote.user.toLowerCase() === voteInfo.name.toLowerCase() -); - -const voteChoice = userInfo ? userInfo.vote : "Not participated"; +const voteChoice = userVote ? userVote.vote : "Not participated";
259-261: PreferObject.hasOwnoverhasOwnPropertyUsing
hasOwnPropertydirectly on potentially user-supplied objects can be hijacked.
Object.hasOwn()is the modern, prototype-safe alternative.- return requiredKeys.every((key) => member.hasOwnProperty(key)); + return requiredKeys.every((key) => Object.hasOwn(member, key));🧰 Tools
🪛 Biome (1.9.4)
[error] 260-260: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/scripts/vote_tracker.js(0 hunks).github/scripts/vote_tracker/index.js(1 hunks).github/scripts/vote_tracker/utils.js(1 hunks)README.md(1 hunks)package.json(1 hunks)test/vote_tracker.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- .github/scripts/vote_tracker.js
🧰 Additional context used
🪛 Biome (1.9.4)
.github/scripts/vote_tracker/index.js
[error] 74-74: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 260-260: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (5)
.github/scripts/vote_tracker/utils.js (1)
1-43: Well-documented function with thorough JSDoc comments!The JSDoc comments are comprehensive, providing clear descriptions of parameters, return values, and good usage examples. This will help other developers understand how to use this function correctly.
README.md (1)
134-161: Clear development documentation added.The new Development section provides clear and helpful instructions for running and creating tests. This will help contributors understand how to properly test their changes, which is essential for maintaining code quality.
test/vote_tracker.test.js (1)
3-91: Well-structured tests covering important edge cases.The test suite is comprehensive and covers several important edge cases:
- Users who haven't voted recently (>90 days)
- Users who have never voted
- Invalid future dates
- New users who just voted
- Inactive members
This thorough testing will help ensure the utility function works correctly in all scenarios.
.github/scripts/vote_tracker/index.js (2)
154-158: Property name mismatch breaks column tooltipThe titles map defines
hasVotedInLast3Months, but objects useisVotedInLast3Months.
As a result the generated markdown shows the raw key instead of the descriptive tooltip.- hasVotedInLast3Months: "Flag indicating if TSC member voted in last 3 months …", + isVotedInLast3Months: "Flag indicating if TSC member voted in last 3 months …",Ensure all writes/readers use the same property name (prefer
hasVotedInLast3Monthsfor readability, or rename everywhere).
After renaming, remember to adjust tests that may assert on this field.
94-103:⚠️ Potential issue
isVotingWithinLastThreeMonthsis evaluated before the record is populated
lastParticipatedVoteTimeis added afterisVotingWithinLastThreeMonths(voteInfo)is called, so the utility receives stale/empty data and will always returnfalse.
Populate the field first, then call the helper.- voteInfo.isVotedInLast3Months = isVotingWithinLastThreeMonths(voteInfo); - voteInfo.lastParticipatedVoteTime = currentTime; +voteInfo.lastParticipatedVoteTime = currentTime; +voteInfo.isVotedInLast3Months = isVotingWithinLastThreeMonths(voteInfo);Likely an incorrect or invalid review comment.
| if (userVote && userVote.timestamp) { | ||
| currentTime = userVote.timestamp.toString().split(" ")[0]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against undefined timestamps with optional-chaining
userVote?.timestamp is safer than directly accessing userVote.timestamp, because userVote is undefined when a TSC member did not cast a binding vote.
Without the guard, the script will throw TypeError: Cannot read properties of undefined (reading 'timestamp'), terminating the workflow prematurely.
- if (userVote && userVote.timestamp) {
- currentTime = userVote.timestamp.toString().split(" ")[0];
- }
+if (userVote?.timestamp) {
+ currentTime = userVote.timestamp.split(" ")[0];
+}📝 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.
| if (userVote && userVote.timestamp) { | |
| currentTime = userVote.timestamp.toString().split(" ")[0]; | |
| } | |
| if (userVote?.timestamp) { | |
| currentTime = userVote.timestamp.split(" ")[0]; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Apply the suggested code change
sed -i '/if (userVote && userVote.timestamp) {/,+2c\ if (userVote?.timestamp) {\n currentTime = userVote.timestamp.split(" ")[0];\n }' .github/scripts/vote_tracker/index.js
# Verify the change was applied correctly
grep -A 2 "if (userVote?.timestamp)" .github/scripts/vote_tracker/index.js
Length of output: 365
@derberg I've applied the suggested change. The code now uses optional chaining for safer access to the timestamp property and removes the unnecessary .toString() call:
if (userVote?.timestamp) {
currentTime = userVote.timestamp.split(" ")[0];
}The change has been committed successfully.
🧬 Code Graph Analysis Results
Relevant snippet from .github/scripts/vote_tracker/index.js (entire file, lines 1-237)
The entire file .github/scripts/vote_tracker/index.js is relevant as it contains the main logic for tracking votes, parsing vote comments, updating vote tracking files, and generating markdown summaries. Key points include:
- Imports:
js-yaml,fs.promises,path, and a utility functionisVotingWithinLastThreeMonths. - Main exported async function accepting
{ github, context, botCommentURL }. - Logic to extract vote comment details either from a bot comment URL or from the GitHub Actions context.
- Parsing vote-closed comments to extract voting rows.
- Reading
MAINTAINERS.yamlto get maintainer information. - Updating vote tracking file (
voteTrackingFile.json) with TSC members and their voting details. - Processing votes to update counts for agree, disagree, abstain, and non-participation.
- Writing updated vote details back to JSON and generating a markdown table (
TSC_VOTING_OVERVIEW.md). - Helper functions:
jsonToMarkdownTable(data): Converts JSON vote data to a markdown table with tooltips and links.parseVoteClosedComment(): Extracts vote rows from the comment message.updateVoteTrackingFile(): Updates the vote tracking file with new TSC members and initializes their vote data.fetchCommentInformation(): Fetches comment and issue details from GitHub API when triggered manually.
- Extensive error handling and logging throughout.
Possibly relevant snippets from other files
-
.github/workflows/create-event-helpers/calendar/index.jsline 135:const currentTime = new Date(Date.now()).toISOString();
This snippet shows a way to get the current ISO timestamp, which is similar to how timestamps are handled in the vote tracker script.
-
.github/scripts/maintainers/gh_calls.jsline 116:const data = await getCodeownersFile(github, owner, repo.name, core);
This snippet indicates a call to fetch CODEOWNERS file data, which might relate to maintainer information, but is not directly connected to vote tracking.
No other snippets appear directly relevant to understanding or modifying .github/scripts/vote_tracker/index.js.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
@derberg can you please review this |
| ### Adding New Tests | ||
| If you are contributing new automation scripts, ensure they are covered by tests. To add a new test: | ||
|
|
||
| 1. Create or update the relevant test file in the `testing/` directory. |
There was a problem hiding this comment.
always remember to update docs 😛
| abstainCount: 2, | ||
| notParticipatingCount: 0 | ||
| }, | ||
| false |
There was a problem hiding this comment.
this test is giving true instead of expecting false
There was a problem hiding this comment.
it;s because you're checking agains the current date, which yeah, now is may, not before april
you need to somehow mock current date and set it as older, before april
|
@Nandgopal-R hey man, did you have a chance to look into my review? |
|
Yes, I did. I'm almost finished—just tidying up the README, after which I'll commit. |
@derberg Yes, that's correct. The only modification to index.js was extracting one function. |
Desciption
Tests Introduced
isVotingWithinLastThreeMonths.Fixes
isVotingWithinLastThreeMonthsto correctly handle future dates given inlastVoteDate.How to test
npm testto execute all test cases.This PR is related to issue #1786
Summary by CodeRabbit
New Features
Documentation
Tests
Chores