test: add pure-logic unit tests + coverage ratchet gate (Phase 1) - #3363
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughAdds ten new Jest test suites covering the Redux store module, critical error handlers, Outlook error classification, logging deduplication/privacy/context utilities, navigation/servers/updates reducers, i18n interpolation formatters, and the desktop capturer cache. A ChangesUnit Test Coverage Expansion
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/errors/main/criticalError.spec.ts`:
- Line 1: The test file criticalError.spec.ts is a main process test but does
not follow the required naming convention. Rename the file from
criticalError.spec.ts to criticalError.main.spec.ts to match the repository's
test naming pattern for main process tests, which requires the format
*.main.spec.ts.
- Around line 283-295: The test for setupRendererErrorHandling only restores the
original BUGSNAG_API_KEY value on the success path, which means if an assertion
fails, the environment variable remains deleted and causes test pollution for
subsequent tests. Wrap the test logic (including the await expect call) in a try
block and move the environment variable restoration into a finally block to
ensure cleanup always happens regardless of whether the test passes or fails.
In `@src/logging/main/context.main.spec.ts`:
- Around line 27-34: The afterEach cleanup function always deletes
globalThis.window, which can cause test pollution if window already existed
before the test suite ran. Similar to how originalType is preserved for
process.type, you should capture the original value of globalThis.window before
any test modifications (likely in a beforeEach or at the top of the describe
block), and then restore that original value in the afterEach cleanup instead of
unconditionally deleting it. This ensures that if window existed before the
tests, it will be properly restored to its pre-test state.
In `@src/servers/reducers/__tests__/servers.spec.ts`:
- Around line 66-74: The immutability assertion in the upsert update test
compares newState to a newly created array literal `[existing]`, which defeats
the purpose of validating that the reducer returns a different array reference
than the input. Store the input array passed to the servers function as a
separate variable, then use that variable reference in the `.not.toBe()`
assertion instead of creating a new array literal in the expect statement. This
will properly verify that the reducer creates a new array instance rather than
mutating the original input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e20f0dc1-6e0d-45bd-b23b-9f7be170b63b
📒 Files selected for processing (12)
jest.config.jssrc/errors/main/criticalError.spec.tssrc/i18n/__tests__/common.spec.tssrc/logging/__tests__/dedup.spec.tssrc/logging/__tests__/privacy.spec.tssrc/logging/main/context.main.spec.tssrc/navigation/reducers/__tests__/navigation.spec.tssrc/outlookCalendar/__tests__/errorClassification.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.tssrc/servers/reducers/__tests__/servers.spec.tssrc/store/__tests__/index.spec.tssrc/updates/reducers/__tests__/updates.spec.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use TypeScript for all new code unless explicitly told otherwise
Use optional chaining with fallbacks for platform-specific APIs instead of mocking when possible. Example:const uid = process.getuid?.() ?? 1000;
Files:
src/i18n/__tests__/common.spec.tssrc/logging/main/context.main.spec.tssrc/navigation/reducers/__tests__/navigation.spec.tssrc/logging/__tests__/dedup.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.tssrc/outlookCalendar/__tests__/errorClassification.spec.tssrc/errors/main/criticalError.spec.tssrc/updates/reducers/__tests__/updates.spec.tssrc/logging/__tests__/privacy.spec.tssrc/store/__tests__/index.spec.tssrc/servers/reducers/__tests__/servers.spec.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed
Import UI components from@rocket.chat/fuselageand checkTheme.d.tsfor valid color tokens
Use React functional components with hooks
Use PascalCase for component file names
Files:
src/i18n/__tests__/common.spec.tssrc/logging/main/context.main.spec.tssrc/navigation/reducers/__tests__/navigation.spec.tssrc/logging/__tests__/dedup.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.tssrc/outlookCalendar/__tests__/errorClassification.spec.tssrc/errors/main/criticalError.spec.tssrc/updates/reducers/__tests__/updates.spec.tssrc/logging/__tests__/privacy.spec.tssrc/store/__tests__/index.spec.tssrc/servers/reducers/__tests__/servers.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfile naming for Renderer process tests
Files:
src/i18n/__tests__/common.spec.tssrc/logging/main/context.main.spec.tssrc/navigation/reducers/__tests__/navigation.spec.tssrc/logging/__tests__/dedup.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.tssrc/outlookCalendar/__tests__/errorClassification.spec.tssrc/errors/main/criticalError.spec.tssrc/updates/reducers/__tests__/updates.spec.tssrc/logging/__tests__/privacy.spec.tssrc/store/__tests__/index.spec.tssrc/servers/reducers/__tests__/servers.spec.ts
**/*.{spec.ts,main.spec.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Only mock platform-specific APIs when defensive coding isn't possible. Linux-only APIs requiring mocks:
process.getuid(),process.getgid(),process.geteuid(),process.getegid()
Files:
src/i18n/__tests__/common.spec.tssrc/logging/main/context.main.spec.tssrc/navigation/reducers/__tests__/navigation.spec.tssrc/logging/__tests__/dedup.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.tssrc/outlookCalendar/__tests__/errorClassification.spec.tssrc/errors/main/criticalError.spec.tssrc/updates/reducers/__tests__/updates.spec.tssrc/logging/__tests__/privacy.spec.tssrc/store/__tests__/index.spec.tssrc/servers/reducers/__tests__/servers.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Redux actions must follow FSA (Flux Standard Action) pattern
Avoid unnecessary comments — write self-documenting code through clear naming
Always verify libraries by checking official docs and.d.tsfiles innode_modules/. Never assume props, tokens, or APIs work without verification
Avoid subjective descriptors ('smart', 'excellent', 'dumb') in documentation and comments
Use measurable descriptions in code documentation: 'reduced memory usage', 'improved by X%' instead of subjective claims
NEVER invent metrics — don't include estimated time spent or speculated user counts. Only include numbers from actual logs, error messages, or documented sources
Files:
src/i18n/__tests__/common.spec.tssrc/logging/main/context.main.spec.tssrc/navigation/reducers/__tests__/navigation.spec.tssrc/logging/__tests__/dedup.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.tssrc/outlookCalendar/__tests__/errorClassification.spec.tssrc/errors/main/criticalError.spec.tssrc/updates/reducers/__tests__/updates.spec.tssrc/logging/__tests__/privacy.spec.tssrc/store/__tests__/index.spec.tssrc/servers/reducers/__tests__/servers.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.main.spec.tsfile naming for Main process tests
Files:
src/logging/main/context.main.spec.tssrc/screenSharing/main/desktopCapturerCache.main.spec.ts
src/outlookCalendar/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/outlookCalendar/AGENTS.md)
src/outlookCalendar/**/*.{ts,tsx}: UsecreateClassifiedError()fromerrorClassification.tsfor user-facing errors to provide error categorization, user-friendly messages, and structured error context
Always use outlookError() for errors as it logs regardless of verbose mode settings, ensuring errors are always visible to users
Files:
src/outlookCalendar/__tests__/errorClassification.spec.ts
🪛 Betterleaks (1.5.0)
src/logging/__tests__/privacy.spec.ts
[high] 62-62: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
[high] 11-11: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 14-14: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 15-15: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 16-16: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 24-24: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 25-25: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 OpenGrep (1.22.0)
src/logging/__tests__/privacy.spec.ts
[ERROR] 103-103: Possible credit card number with dashes or spaces detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.
(coderabbit.pii.credit-card-number-dashed)
[ERROR] 104-104: Possible credit card number (PAN) detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.
(coderabbit.pii.credit-card-number)
[ERROR] 105-105: Possible credit card number (PAN) detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.
(coderabbit.pii.credit-card-number)
[ERROR] 112-112: Possible credit card number (PAN) detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.
(coderabbit.pii.credit-card-number)
[ERROR] 112-112: Possible credit card number (PAN) detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.
(coderabbit.pii.credit-card-number)
🔇 Additional comments (9)
jest.config.js (1)
13-20: LGTM!src/store/__tests__/index.spec.ts (1)
1-437: LGTM!src/i18n/__tests__/common.spec.ts (1)
1-157: LGTM!src/screenSharing/main/desktopCapturerCache.main.spec.ts (1)
1-333: LGTM!src/navigation/reducers/__tests__/navigation.spec.ts (1)
1-216: LGTM!src/updates/reducers/__tests__/updates.spec.ts (1)
1-362: LGTM!src/outlookCalendar/__tests__/errorClassification.spec.ts (1)
1-266: LGTM!src/logging/__tests__/dedup.spec.ts (1)
1-149: LGTM!src/logging/__tests__/privacy.spec.ts (1)
1-258: LGTM!
| @@ -0,0 +1,342 @@ | |||
| /** | |||
There was a problem hiding this comment.
Rename this main-process spec to the required *.main.spec.ts pattern.
This test targets main-process error handling but is named criticalError.spec.ts. Please rename it to criticalError.main.spec.ts to match repository test naming rules.
As per coding guidelines, "**/*.main.spec.ts: Use *.main.spec.ts file naming for Main process tests".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/errors/main/criticalError.spec.ts` at line 1, The test file
criticalError.spec.ts is a main process test but does not follow the required
naming convention. Rename the file from criticalError.spec.ts to
criticalError.main.spec.ts to match the repository's test naming pattern for
main process tests, which requires the format *.main.spec.ts.
Source: Coding guidelines
| it('returns without error when BUGSNAG_API_KEY is not set', async () => { | ||
| const errorsModule = await loadErrorsModule(); | ||
| const original = process.env.BUGSNAG_API_KEY; | ||
| delete process.env.BUGSNAG_API_KEY; | ||
|
|
||
| await expect( | ||
| errorsModule.setupRendererErrorHandling('main') | ||
| ).resolves.toBeUndefined(); | ||
|
|
||
| if (original !== undefined) { | ||
| process.env.BUGSNAG_API_KEY = original; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Use try/finally when mutating process.env.BUGSNAG_API_KEY in tests.
These tests restore env state only on the success path. If an assertion fails, the leaked env var can make later tests flaky.
Suggested fix
it('returns without error when BUGSNAG_API_KEY is not set', async () => {
const errorsModule = await loadErrorsModule();
const original = process.env.BUGSNAG_API_KEY;
- delete process.env.BUGSNAG_API_KEY;
-
- await expect(
- errorsModule.setupRendererErrorHandling('main')
- ).resolves.toBeUndefined();
-
- if (original !== undefined) {
- process.env.BUGSNAG_API_KEY = original;
- }
+ try {
+ delete process.env.BUGSNAG_API_KEY;
+ await expect(
+ errorsModule.setupRendererErrorHandling('main')
+ ).resolves.toBeUndefined();
+ } finally {
+ if (original !== undefined) {
+ process.env.BUGSNAG_API_KEY = original;
+ } else {
+ delete process.env.BUGSNAG_API_KEY;
+ }
+ }
});
it('registers a settings listener when reporting is enabled', async () => {
jest.resetModules();
const original = process.env.BUGSNAG_API_KEY;
- process.env.BUGSNAG_API_KEY = '12345678901234567890123456789012';
+ try {
+ process.env.BUGSNAG_API_KEY = '12345678901234567890123456789012';
- appQuitMock = jest.fn();
- const listenMock = jest.fn(() => () => undefined);
- const startSessionMock = jest.fn();
+ appQuitMock = jest.fn();
+ const listenMock = jest.fn(() => () => undefined);
+ const startSessionMock = jest.fn();
- jest.doMock('electron', () => ({
- app: { quit: appQuitMock, getVersion: jest.fn(() => TEST_APP_VERSION) },
- }));
- jest.doMock('../../store', () => ({
- select: jest.fn(() => ({
- appVersion: TEST_APP_VERSION,
- isReportEnabled: true,
- })),
- listen: listenMock,
- }));
- jest.doMock('`@bugsnag/js`', () => ({
- __esModule: true,
- default: {
- isStarted: jest.fn(() => false),
- notify: jest.fn(),
- start: jest.fn(() => ({
- startSession: startSessionMock,
- pauseSession: jest.fn(),
- })),
- },
- }));
+ jest.doMock('electron', () => ({
+ app: { quit: appQuitMock, getVersion: jest.fn(() => TEST_APP_VERSION) },
+ }));
+ jest.doMock('../../store', () => ({
+ select: jest.fn(() => ({
+ appVersion: TEST_APP_VERSION,
+ isReportEnabled: true,
+ })),
+ listen: listenMock,
+ }));
+ jest.doMock('`@bugsnag/js`', () => ({
+ __esModule: true,
+ default: {
+ isStarted: jest.fn(() => false),
+ notify: jest.fn(),
+ start: jest.fn(() => ({
+ startSession: startSessionMock,
+ pauseSession: jest.fn(),
+ })),
+ },
+ }));
- const errorsModule: ErrorsModule = await import('../../errors');
- await errorsModule.setupRendererErrorHandling('rootWindow');
+ const errorsModule: ErrorsModule = await import('../../errors');
+ await errorsModule.setupRendererErrorHandling('rootWindow');
- expect(listenMock).toHaveBeenCalledWith(
- SETTINGS_SET_REPORT_OPT_IN_CHANGED,
- expect.any(Function)
- );
-
- if (original !== undefined) {
- process.env.BUGSNAG_API_KEY = original;
- } else {
- delete process.env.BUGSNAG_API_KEY;
- }
+ expect(listenMock).toHaveBeenCalledWith(
+ SETTINGS_SET_REPORT_OPT_IN_CHANGED,
+ expect.any(Function)
+ );
+ } finally {
+ if (original !== undefined) {
+ process.env.BUGSNAG_API_KEY = original;
+ } else {
+ delete process.env.BUGSNAG_API_KEY;
+ }
+ }
});Also applies to: 297-340
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/errors/main/criticalError.spec.ts` around lines 283 - 295, The test for
setupRendererErrorHandling only restores the original BUGSNAG_API_KEY value on
the success path, which means if an assertion fails, the environment variable
remains deleted and causes test pollution for subsequent tests. Wrap the test
logic (including the await expect call) in a try block and move the environment
variable restoration into a finally block to ensure cleanup always happens
regardless of whether the test passes or fails.
| afterEach(() => { | ||
| Object.defineProperty(process, 'type', { | ||
| value: originalType, | ||
| configurable: true, | ||
| }); | ||
| // Clean up any window stub left on globalThis. | ||
| delete (globalThis as any).window; | ||
| }); |
There was a problem hiding this comment.
Restore any pre-existing globalThis.window instead of always deleting it.
The current cleanup always deletes window. If this suite runs where window already exists, that global can leak into other suites as a destructive side effect.
Suggested fix
describe('logging/context', () => {
describe('getProcessContext', () => {
const originalType = process.type;
+ const originalWindow = (globalThis as any).window;
afterEach(() => {
Object.defineProperty(process, 'type', {
value: originalType,
configurable: true,
});
- // Clean up any window stub left on globalThis.
- delete (globalThis as any).window;
+ if (originalWindow === undefined) {
+ delete (globalThis as any).window;
+ } else {
+ (globalThis as any).window = originalWindow;
+ }
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/logging/main/context.main.spec.ts` around lines 27 - 34, The afterEach
cleanup function always deletes globalThis.window, which can cause test
pollution if window already existed before the test suite ran. Similar to how
originalType is preserved for process.type, you should capture the original
value of globalThis.window before any test modifications (likely in a beforeEach
or at the top of the describe block), and then restore that original value in
the afterEach cleanup instead of unconditionally deleting it. This ensures that
if window existed before the tests, it will be properly restored to its pre-test
state.
| it('should merge into an existing server (upsert update path)', () => { | ||
| const newState = servers([existing], { | ||
| type: ADD_SERVER_VIEW_SERVER_ADDED, | ||
| payload: url, | ||
| } as any); | ||
|
|
||
| expect(newState).toEqual([{ url, title: url }]); | ||
| expect(newState).not.toBe([existing]); | ||
| }); |
There was a problem hiding this comment.
Fix ineffective immutability assertion in upsert update test.
Line 73 compares newState to a new literal array, so the assertion is always true and doesn't validate reducer behavior. Compare against the actual input array reference instead.
Suggested patch
it('should merge into an existing server (upsert update path)', () => {
- const newState = servers([existing], {
+ const state = [existing];
+ const newState = servers(state, {
type: ADD_SERVER_VIEW_SERVER_ADDED,
payload: url,
} as any);
expect(newState).toEqual([{ url, title: url }]);
- expect(newState).not.toBe([existing]);
+ expect(newState).not.toBe(state);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/servers/reducers/__tests__/servers.spec.ts` around lines 66 - 74, The
immutability assertion in the upsert update test compares newState to a newly
created array literal `[existing]`, which defeats the purpose of validating that
the reducer returns a different array reference than the input. Store the input
array passed to the servers function as a separate variable, then use that
variable reference in the `.not.toBe()` assertion instead of creating a new
array literal in the expect statement. This will properly verify that the
reducer creates a new array instance rather than mutating the original input.
Adds 11 unit-test specs covering pure-logic modules that had little or no coverage: Redux reducers (servers, updates, navigation), log redaction and dedup, Outlook error classification, i18n formatters, store helpers, critical-error matcher, logging context, and the desktop-capturer cache. Each target reaches ~94-100% line coverage. Tests follow existing repo conventions (reducer-direct, it.each matrices, extract-logic-from-IPC); specs are placed in subdirs to satisfy the jest testMatch globs. Adds a coverageThreshold gate to jest.config.js (lines/statements 21, branches 19, functions 15) so coverage cannot regress below the new baseline. validate-pr.yml already runs test:coverage, so the gate is active in CI. Coverage: lines 17.15% -> 22.34%, statements 17.67% -> 22.91%, branches 13.18% -> 20.39%, functions 11.87% -> 16.11%. 640 tests pass.
54013d1 to
c46c552
Compare
1 participant
What
Phase 1 of the coverage-improvement plan: unit tests for high-ROI pure-logic modules that had little or no coverage, plus a
coverageThresholdratchet gate so the number can't regress.Coverage delta
640 tests pass, 2 skipped, 0 failures. Lint clean.
New specs (11 files, ~94–100% on each target)
servers/reducers.tsservers/reducers/__tests__/servers.spec.tsupdates/reducers.tsupdates/reducers/__tests__/updates.spec.tsnavigation/reducers.tsnavigation/reducers/__tests__/navigation.spec.tslogging/privacy.tslogging/__tests__/privacy.spec.tslogging/dedup.tslogging/__tests__/dedup.spec.tslogging/context.tslogging/main/context.main.spec.tsoutlookCalendar/errorClassification.tsoutlookCalendar/__tests__/errorClassification.spec.tsi18n/common.tsi18n/__tests__/common.spec.tsstore/index.tsstore/__tests__/index.spec.tserrors.tserrors/main/criticalError.spec.tsscreenSharing/desktopCapturerCache.tsscreenSharing/main/desktopCapturerCache.main.spec.tsConventions followed
Reuses the three existing repo test patterns — reducer-direct (
isMenuBarEnabled.spec.ts),it.eachinput/output matrices (getOutlookEvents.spec.ts), and extract-logic-from-IPC (videoCallWindow/main/ipc.spec.ts). No new test harness.Specs are placed in subdirs (
__tests__/,main/) to satisfy the jesttestMatchglobs — flatsrc/<dir>/*.spec.tsis silently dropped by the renderer glob. Each spec was confirmed discoverable viajest --listTestsbefore landing.Ratchet gate
Adds
coverageThresholdtojest.config.js(lines/statements 21, branches 19, functions 15 — set just under the new baseline for cross-platform headroom).validate-pr.ymlalready runstest:coverage, so the gate is active in CI and fails any PR that drops coverage below the floor.Notes
throwguard inservers/reducers.ts, process-undefined guards) rather than contriving synthetic inputs. No source files were modified.Next (deferred)
Phase 2 — extract-and-test the moderate files (
navigation/main.tscert utils,ScreenSharingRequestTracker,browserLauncher,ipc/renderer,logging/index.ts), then ratchet the threshold up again.Summary by CodeRabbit
Tests
Chores