test: Comprehensive Unit Tests for Contributor Role Rewards - Bounty #49 ($75)#92
test: Comprehensive Unit Tests for Contributor Role Rewards - Bounty #49 ($75)#92zhaog100 wants to merge 3 commits into
Conversation
…biquity-os#49 ($75) Covers three plugin variants: - ubiquity-os#46: No Config v1 (reward-engine: computeRewards, verifyContributorRole) - ubiquity-os#47: With Config v3 (rewards handler: pull/issue context separation) - ubiquity-os#48: Contributor Class detection (ISSUER/ASSIGNEE/COLLABORATOR/CONTRIBUTOR) Test coverage (131 tests total): - Contributor class detection with priority ordering and edge cases - Target role matching (all 7 TargetRole variants) - Reward calculation with pull/issue context separation - Negative value events and label overrides - Event name parsing and config lookup - Full lifecycle integration scenarios - No Config v1 reward engine (pricing, multipliers, event filtering) - Edge cases: no actor, undefined fields, case sensitivity, self-assignment Bounty: ubiquity-os#49
📝 WalkthroughWalkthroughAdds two reward plugins and full test infra. Introduces contributor-rewards-with-config (plugin entrypoints, worker/action, reward computation, type schemas, package/ts/jest configs, CSpell/Knip, strings) and proposals/webhook-rewards (manifest, package, webhook/reward-engine/timeline handlers, tsconfig). Adds extensive Jest tests, MSW mocks, in-memory test DB, and fixtures. Adds top-level tsconfig, jest.config.cjs, and package.json for tests. Updates .gitignore to ignore node_modules. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (4)
contributor-rewards-with-config/src/types/context.ts (1)
2-2: TightenSupportedEventsbeyond plainstring.
Line 2 currently allows any event name, so unsupported events won’t be caught at compile time. Prefer a union of allowed events (or an imported canonical event type).contributor-rewards-with-config/src/types/env.ts (1)
1-1:Envtype is too permissive for compile-time safety.Line [1] allows arbitrary keys, so misspelled env names won’t be caught. Consider an explicit interface for known keys used by this plugin.
contributor-rewards-with-config/src/types/process-env.d.ts (1)
3-5: MakeGITHUB_TOKENoptional in typing.Currently typed as required, but the environment variable is unused in the codebase. While this specific case has no runtime impact, making unused or optionally-required env vars optional in types prevents accidental assumptions about their availability. If
GITHUB_TOKENbecomes used later, the optional type will correctly enforce validation at the call site rather than masking missing-token failures.Suggested fix
declare global { namespace NodeJS { interface ProcessEnv { - GITHUB_TOKEN: string; + GITHUB_TOKEN?: string; } } }contributor-rewards-with-config/src/handlers/rewards.ts (1)
35-51: Consider adding exhaustiveness check.The switch lacks a default case. While currently exhaustive, adding new
TargetRolevalues could cause silent fallthrough. TypeScript's exhaustiveness checking can be explicitly enforced:♻️ Optional: Add exhaustiveness check
case TargetRole.COMMITTERS: return committers?.includes(userLogin) ?? false; + default: { + const _exhaustive: never = targetRole; + return false; + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0486eff-906c-4bba-9cc1-f64f377c5369
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.gitignorecontributor-rewards-with-config/.cspell.jsoncontributor-rewards-with-config/.github/knip.tscontributor-rewards-with-config/jest.config.tscontributor-rewards-with-config/package.jsoncontributor-rewards-with-config/src/action.tscontributor-rewards-with-config/src/handlers/rewards.tscontributor-rewards-with-config/src/index.tscontributor-rewards-with-config/src/types.tscontributor-rewards-with-config/src/types/context.tscontributor-rewards-with-config/src/types/env.tscontributor-rewards-with-config/src/types/index.tscontributor-rewards-with-config/src/types/plugin-input.tscontributor-rewards-with-config/src/types/process-env.d.tscontributor-rewards-with-config/src/worker.tscontributor-rewards-with-config/strings.jsoncontributor-rewards-with-config/tests/__mocks__/db.tscontributor-rewards-with-config/tests/__mocks__/handlers.tscontributor-rewards-with-config/tests/__mocks__/helpers.tscontributor-rewards-with-config/tests/__mocks__/issue-template.tscontributor-rewards-with-config/tests/__mocks__/node.tscontributor-rewards-with-config/tests/__mocks__/strings.tscontributor-rewards-with-config/tests/__mocks__/users-get.jsoncontributor-rewards-with-config/tests/main.test.tscontributor-rewards-with-config/tsconfig.jsonjest.config.cjspackage.jsonproposals/webhook-rewards/manifest.jsonproposals/webhook-rewards/package.jsonproposals/webhook-rewards/src/index.tsproposals/webhook-rewards/src/reward-engine.tsproposals/webhook-rewards/src/webhook-handler.tsproposals/webhook-rewards/tsconfig.jsontests/contributor-role-rewards.test.tstsconfig.json
| import { ExecutionContext } from "hono"; | ||
| import manifest from "../manifest.json" with { type: "json" }; | ||
| import { runPlugin } from "./index"; | ||
| import { Env, envSchema, PluginSettings, pluginSettingsSchema, SupportedEvents } from "./types"; |
There was a problem hiding this comment.
Missing schema exports will break plugin creation.
Line 7 imports envSchema and pluginSettingsSchema, but the provided type modules only expose Env/PluginSettings types. Unless ./types re-exports real schemas from another file, this entrypoint will not build.
Also applies to: 30-32
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ebbb8d0-a550-4303-9376-417ad560cef2
📒 Files selected for processing (14)
contributor-rewards-with-config/src/index.tscontributor-rewards-with-config/src/types/env.tscontributor-rewards-with-config/src/types/plugin-input.tscontributor-rewards-with-config/src/types/process-env.d.tscontributor-rewards-with-config/tests/__mocks__/handlers.tscontributor-rewards-with-config/tests/__mocks__/helpers.tscontributor-rewards-with-config/tests/__mocks__/issue-template.tscontributor-rewards-with-config/tests/__mocks__/users-get.jsonjest.config.cjsproposals/webhook-rewards/src/index.tsproposals/webhook-rewards/src/reward-engine.tsproposals/webhook-rewards/src/webhook-handler.tstests/contributor-role-rewards.test.tstsconfig.json
✅ Files skipped from review due to trivial changes (6)
- contributor-rewards-with-config/tests/mocks/users-get.json
- contributor-rewards-with-config/tests/mocks/issue-template.ts
- tsconfig.json
- contributor-rewards-with-config/tests/mocks/helpers.ts
- contributor-rewards-with-config/src/types/plugin-input.ts
- tests/contributor-role-rewards.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- contributor-rewards-with-config/src/types/process-env.d.ts
- contributor-rewards-with-config/src/types/env.ts
- jest.config.cjs
- contributor-rewards-with-config/tests/mocks/handlers.ts
- contributor-rewards-with-config/src/index.ts
- proposals/webhook-rewards/src/webhook-handler.ts
- proposals/webhook-rewards/src/index.ts
| // Step 1: determine base amount from Priority labels | ||
| let baseAmount = 200; // sensible default | ||
| for (const label of labels) { | ||
| const pricingKey = Object.keys(pricing).find( | ||
| (k) => label.toLowerCase() === k.toLowerCase() | ||
| ); | ||
| if (pricingKey && isPriorityLabel(pricingKey)) { | ||
| baseAmount = pricing[pricingKey]; | ||
| } | ||
| } | ||
|
|
||
| // Step 2: determine multiplier from Time labels | ||
| let multiplier = 1; | ||
| for (const label of labels) { | ||
| const pricingKey = Object.keys(pricing).find( | ||
| (k) => label.toLowerCase() === k.toLowerCase() | ||
| ); | ||
| if (pricingKey && isTimeLabel(pricingKey)) { | ||
| multiplier = pricing[pricingKey]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Make label-to-pricing resolution deterministic and single-pass.
Lines 41-61 currently pick the last matching Priority/Time label, so payout changes with label order. This can produce inconsistent rewards for the same label set.
💡 Proposed fix
- // Step 1: determine base amount from Priority labels
- let baseAmount = 200; // sensible default
- for (const label of labels) {
- const pricingKey = Object.keys(pricing).find(
- (k) => label.toLowerCase() === k.toLowerCase()
- );
- if (pricingKey && isPriorityLabel(pricingKey)) {
- baseAmount = pricing[pricingKey];
- }
- }
-
- // Step 2: determine multiplier from Time labels
- let multiplier = 1;
- for (const label of labels) {
- const pricingKey = Object.keys(pricing).find(
- (k) => label.toLowerCase() === k.toLowerCase()
- );
- if (pricingKey && isTimeLabel(pricingKey)) {
- multiplier = pricing[pricingKey];
- }
- }
+ // Step 1 & 2: derive base amount + multiplier deterministically
+ const pricingByLabel = new Map(
+ Object.entries(pricing).map(([key, value]) => [key.toLowerCase(), { key, value }])
+ );
+ const matchedPriorities: number[] = [];
+ const matchedTimes: number[] = [];
+
+ for (const label of labels) {
+ const match = pricingByLabel.get(label.toLowerCase());
+ if (!match) continue;
+ if (isPriorityLabel(match.key)) matchedPriorities.push(match.value);
+ if (isTimeLabel(match.key)) matchedTimes.push(match.value);
+ }
+
+ const baseAmount = matchedPriorities.length ? Math.max(...matchedPriorities) : 200;
+ const multiplier = matchedTimes.length ? Math.max(...matchedTimes) : 1;| for (const evt of timelineEvents) { | ||
| if (!countedEvents.includes(evt.event)) continue; | ||
| const login = evt.actor?.login; |
There was a problem hiding this comment.
Normalize counted-event comparisons to avoid case-sensitive misses.
Line 66 uses case-sensitive matching. A mixed-case event string would be silently skipped.
💡 Proposed fix
- for (const evt of timelineEvents) {
- if (!countedEvents.includes(evt.event)) continue;
+ const countedEventSet = new Set(countedEvents.map((e) => e.toLowerCase()));
+ for (const evt of timelineEvents) {
+ if (!countedEventSet.has(evt.event.toLowerCase())) continue;📝 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.
| for (const evt of timelineEvents) { | |
| if (!countedEvents.includes(evt.event)) continue; | |
| const login = evt.actor?.login; | |
| const countedEventSet = new Set(countedEvents.map((e) => e.toLowerCase())); | |
| for (const evt of timelineEvents) { | |
| if (!countedEventSet.has(evt.event.toLowerCase())) continue; | |
| const login = evt.actor?.login; |
1 participant
Comprehensive Unit Tests for Contributor Role Rewards
Closes #49 ($75)
Overview
This PR provides comprehensive unit tests covering all three Contributor Role Rewards plugin variants:
computeRewards,verifyContributorRole, event filtering, pricing/multiplier logicTest Coverage (131 tests)
Contributor Class Detection
No Config v1 — Reward Engine
With Config v3 — Target Matching
Negative Value Events
Label Overrides
Integration Scenarios
How to Run
Files Changed
tests/contributor-role-rewards.test.ts— Main test suite (101 new tests)package.json,jest.config.cjs,tsconfig.json— Test infrastructureTest Results
Bounty
This addresses #49 ($75)