Skip to content

chore(apps): unit test improvements - #40785

Merged
ggazzo merged 5 commits into
developfrom
chore/apps-tests-improvement
Jun 25, 2026
Merged

chore(apps): unit test improvements#40785
ggazzo merged 5 commits into
developfrom
chore/apps-tests-improvement

Conversation

@d-gubert

@d-gubert d-gubert commented Jun 3, 2026

Copy link
Copy Markdown
Member

Proposed changes (including videos or screenshots)

  • Remove @seald-io/nedb dependency from apps unit tests - unnecessary complexity and eventual flakiness
  • Allow reads and writes to the file system during deno unit tests. The permissions used ended up not working when the OS didn't provide a /tmp temporary directory (e.g. OSX), and permissions in unit tests don't need to be that restricting anyway.

Issue(s)

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • Tests
    • Switched app test storage from a shared NeDB-backed singleton to a per-instance in-memory store.
    • Updated test storage behavior for duplicate detection, missing-record handling (returning null), added clear(), and completed update operations.
  • Chores
    • Updated code ownership rules for app-related paths.
    • Removed an unused development dependency.
  • Developer Experience
    • Simplified local Deno test execution and adjusted test runtime filesystem permissions.

Task: ARCH-2275

@changeset-bot

changeset-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: b1e5eba

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Migrate TestsAppStorage from a NeDB-backed singleton to an in-memory Map per instance; remove the NeDB devDependency, broaden Deno test permissions, update CODEOWNERS, and wire tests to instantiate storage directly.

Changes

Test Storage Migration

Layer / File(s) Summary
Configuration and dependency cleanup
packages/apps/package.json, packages/apps/deno-runtime/deno.jsonc, .github/CODEOWNERS
Removed @seald-io/nedb from devDependencies; changed the Deno test task from path-restricted read/write flags to unrestricted --allow-read and --allow-write; added CODEOWNERS rules for /packages/apps/ and the Enterprise app directories.
In-memory storage implementation
packages/apps/tests/test-data/storage/storage.ts
Rewrote TestsAppStorage to use a private Map<string, IAppStorageItem> with a public constructor and super('in-memory'); implemented create, retrieveOne, retrieveAll, retrieveAllPrivate, clear, remove, and the update methods against the Map store.
Test infrastructure wiring
packages/apps/tests/test-data/utilities.ts
TestInfastructureSetup now assigns this.appStorage = new TestsAppStorage() instead of using TestsAppStorage.getInstance().

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • tassoevan
  • yash-rajpal
  • ggazzo
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and broadly matches the PR’s apps unit test changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@dionisio-bot

dionisio-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@d-gubert
d-gubert force-pushed the chore/apps-tests-improvement branch from d754717 to 5ddb6de Compare June 3, 2026 19:14
@codecov

codecov Bot commented Jun 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.10%. Comparing base (660215a) to head (b1e5eba).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #40785      +/-   ##
===========================================
- Coverage    70.11%   70.10%   -0.02%     
===========================================
  Files         3363     3363              
  Lines       129716   129716              
  Branches     22464    22489      +25     
===========================================
- Hits         90953    90939      -14     
- Misses       35442    35454      +12     
- Partials      3321     3323       +2     
Flag Coverage Δ
e2e 59.36% <ø> (+0.02%) ⬆️
e2e-api 46.30% <ø> (-0.85%) ⬇️
unit 70.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@d-gubert
d-gubert marked this pull request as ready for review June 5, 2026 16:02
@d-gubert
d-gubert requested a review from a team as a code owner June 5, 2026 16:02

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 5 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/apps/tests/test-data/storage/storage.ts Outdated
Comment thread packages/apps/tests/test-data/storage/storage.ts Outdated
@d-gubert

d-gubert commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

@copilot
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.

P1: `create()` should populate `_id` on the stored record. Without it, callers that depend on `created._id` lose the storage identifier and later app update/private-API flows in tests can break. P1: `updatePartialAndReturnDocument` incorrectly requires `id` and does not handle `_id`-only updates used by AppManager.

Copilot AI commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

@copilot
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.

...

Both reported issues were valid and shared the same root cause in the in-memory test storage implementation:

  • create() didn’t guarantee _id, so downstream flows relying on created._id could break.
  • updatePartialAndReturnDocument() only accepted id, while AppManager can pass _id-only partial updates.

Fixed in commit 547ab51 by (1) always setting _id: item._id ?? item.id in create(), and (2) resolving updates by item.id ?? item._id in updatePartialAndReturnDocument().

@d-gubert
d-gubert force-pushed the chore/apps-tests-improvement branch from 547ab51 to 2984037 Compare June 19, 2026 14:29
sampaiodiego
sampaiodiego previously approved these changes Jun 19, 2026
@d-gubert d-gubert added the stat: QA assured Means it has been tested and approved by a company insider label Jun 19, 2026
@d-gubert d-gubert added this to the 8.6.0 milestone Jun 19, 2026
@dionisio-bot dionisio-bot Bot added the stat: ready to merge PR tested and approved waiting for merge label Jun 19, 2026
@dionisio-bot
dionisio-bot Bot added this pull request to the merge queue Jun 19, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 19, 2026
@dionisio-bot
dionisio-bot Bot added this pull request to the merge queue Jun 19, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 19, 2026
@alfredodelfabro alfredodelfabro modified the milestone: 8.6.0 Jun 20, 2026
@dionisio-bot dionisio-bot Bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Jun 20, 2026
@dionisio-bot
dionisio-bot Bot added this pull request to the merge queue Jun 20, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 20, 2026
@dionisio-bot
dionisio-bot Bot added this pull request to the merge queue Jun 20, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 20, 2026
@dionisio-bot
dionisio-bot Bot added this pull request to the merge queue Jun 20, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 20, 2026
@d-gubert d-gubert modified the milestones: 8.6.0, 8.7.0 Jun 22, 2026
@dionisio-bot dionisio-bot Bot removed the stat: ready to merge PR tested and approved waiting for merge label Jun 22, 2026
@d-gubert
d-gubert force-pushed the chore/apps-tests-improvement branch 2 times, most recently from f069525 to 4c3f573 Compare June 25, 2026 14:26
@d-gubert
d-gubert force-pushed the chore/apps-tests-improvement branch from f02a919 to b1e5eba Compare June 25, 2026 16:52
d-gubert added a commit that referenced this pull request Jun 25, 2026
commit b1e5eba
Author: Douglas Gubert <douglas.gubert@gmail.com>
Date:   Thu Jun 25 13:07:19 2026 -0300

    test(apps): clean deno-runtime temp link before running

commit c39875e
Author: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Date:   Fri Jun 5 17:57:10 2026 +0000

    test(apps): preserve _id and support _id partial updates in test storage

commit 114e5e7
Author: Douglas Gubert <douglas.gubert@gmail.com>
Date:   Wed Jun 3 16:37:07 2026 -0300

    chore: add @RocketChat/apps as codeowner of more paths

commit 28a45be
Author: Douglas Gubert <douglas.gubert@gmail.com>
Date:   Wed Jun 3 15:54:13 2026 -0300

    chore(apps): loosen permissions for deno test

commit 1895ed4
Author: Douglas Gubert <douglas.gubert@gmail.com>
Date:   Wed Jun 3 15:46:33 2026 -0300

    chore(apps): replace nedb in tests for custom mock

    fix mock

commit 660215a
Author: Ricardo Garim <rswarovsky@gmail.com>
Date:   Thu Jun 25 11:06:59 2026 -0300

    chore: use @rocket.chat/logger in presence service (#41034)
d-gubert added a commit that referenced this pull request Jun 25, 2026
commit b1e5eba
Author: Douglas Gubert <douglas.gubert@gmail.com>
Date:   Thu Jun 25 13:07:19 2026 -0300

    test(apps): clean deno-runtime temp link before running

commit c39875e
Author: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Date:   Fri Jun 5 17:57:10 2026 +0000

    test(apps): preserve _id and support _id partial updates in test storage

commit 114e5e7
Author: Douglas Gubert <douglas.gubert@gmail.com>
Date:   Wed Jun 3 16:37:07 2026 -0300

    chore: add @RocketChat/apps as codeowner of more paths

commit 28a45be
Author: Douglas Gubert <douglas.gubert@gmail.com>
Date:   Wed Jun 3 15:54:13 2026 -0300

    chore(apps): loosen permissions for deno test

commit 1895ed4
Author: Douglas Gubert <douglas.gubert@gmail.com>
Date:   Wed Jun 3 15:46:33 2026 -0300

    chore(apps): replace nedb in tests for custom mock

    fix mock

commit 660215a
Author: Ricardo Garim <rswarovsky@gmail.com>
Date:   Thu Jun 25 11:06:59 2026 -0300

    chore: use @rocket.chat/logger in presence service (#41034)
@ggazzo
ggazzo merged commit 64af832 into develop Jun 25, 2026
46 of 47 checks passed
@ggazzo
ggazzo deleted the chore/apps-tests-improvement branch June 25, 2026 17:57
@ggazzo

ggazzo commented Jul 20, 2026

Copy link
Copy Markdown
Member

/jira ARCH-2228

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider type: chore

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants