test(structures): add tests for several structures#11407
test(structures): add tests for several structures#11407AsadHumayun wants to merge 47 commits intodiscordjs:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11407 +/- ##
==========================================
+ Coverage 31.67% 37.46% +5.79%
==========================================
Files 388 387 -1
Lines 14010 14009 -1
Branches 1106 1103 -3
==========================================
+ Hits 4437 5248 +811
+ Misses 9435 8583 -852
- Partials 138 178 +40
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AsadHumayun
force-pushed
the
feat/add-tests-several-structures
branch
from
9edce03 to
ac342cd
Compare
Qjuh
left a comment
Qjuh
left a comment
There was a problem hiding this comment.
Apart from these little nitpicks LGTM
This was mostly written by Qjuh in all honesty and I went and applied their review comments. This commit aims to address review comments on this PR. -- Co-authored-by: Qjuh <76154676+Qjuh@users.noreply.github.com>
|
Hi there, I think this PR covers enough structures in tests that it's ready for review/merge now. Apologies it has taken so long to get round to this. I've applied all comments that standardised testing approaches for the dates/timestamps. I didn't apply changes to rename them to the |
📝 WalkthroughWalkthroughThis PR adds comprehensive Vitest test coverage for multiple Discord structures (AutoModerationRule, Emoji, Entitlement, message components/embeds, Poll, SKU, SoundboardSound, StageInstance, Sticker, Subscription, Team, User, Voice, Webhook) and their nested sub-structures. Tests validate property getters, toJSON() serialization, and in-place patching behavior. Source changes include adding SKU timestamp getters, optimizing VoiceState's request-to-speak timestamp handling, and updating ButtonComponent's disabled field validation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 18
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/structures/__tests__/automoderation.test.ts`:
- Line 17: Update the ESM import for the kPatch symbol to include the .js
extension; locate the import line that reads "import { kPatch } from
'../src/utils/symbols';" in the tests and change it to import from
'../src/utils/symbols.js' so it matches other imports and avoids ESM resolution
issues.
- Around line 94-102: The assertion currently compares patched.mentionTotalLimit
to itself which always passes; change it to assert the actual expected patched
value (e.g., expect(patched.mentionTotalLimit).toBe(10)) and optionally assert
the patch occurred in place by checking identity or equality with the original
instance (e.g., expect(patched).toBe(instance) or
expect(patched.toJSON()).toEqual(instance.toJSON())); update the test around
instance[kPatch], patched, and mentionTotalLimit accordingly.
- Around line 117-125: The test incorrectly asserts instance.type equals
patched.type after calling instance[kPatch](...), which always passes because
the patch mutates the same object; change the assertion to compare instance.type
(or patched.type) against the expected enum value
AutoModerationActionType.Timeout (or its numeric/string representation) instead.
Locate the test using the kPatch call (function symbol kPatch and variable names
instance and patched) and replace the faulty
expect(instance.type).toBe(patched.type) with an assertion like
expect(instance.type).toBe(AutoModerationActionType.Timeout), keeping the
existing toJSON equality check if desired.
In `@packages/structures/__tests__/entitlement.test.ts`:
- Line 21: The test creates a shared Entitlement instance (const instance = new
Entitlement(data)) at describe scope and one test mutates it, causing test-order
dependency; fix by moving creation into a per-test lifecycle (use beforeEach to
re-instantiate the Entitlement) so each test gets a fresh instance, and add the
beforeEach import from vitest; update references that currently use the
top-level instance to use the instance provided by beforeEach (e.g., the same
variable name scoped to the describe).
In `@packages/structures/__tests__/message.components.test.ts`:
- Line 295: The test title string in the unit test "Patching the
ThumbnailCompoonent works in place" contains a typo; update the test description
in the test(...) call to "Patching the ThumbnailComponent works in place" so the
component name is spelled correctly (look for the test case declaration near the
spec that references ThumbnailCompoonent and change the string literal to
ThumbnailComponent).
- Line 61: Update the import of the kPatch symbol to use the ESM-compatible file
extension by changing the import statement that currently reads "import { kPatch
} from '../src/utils/symbols';" to include the ".js" extension so it becomes
"../src/utils/symbols.js"; locate the import of kPatch in the test file and
adjust only the module path string to append ".js".
- Around line 946-973: Remove the duplicate test suite for TextDisplayComponent
by deleting the redundant describe block that constructs a new
TextDisplayComponent from textDisplayComponent and runs the same assertions
(including checks against content, type, id, toJSON(), and the kPatch behavior);
keep the original suite (the first occurrence) so TextDisplayComponent,
textDisplayComponent, kPatch, and toJSON remain covered and ensure no other
tests reference the removed block.
- Around line 247-251: Rename the misspelled test description strings
"UserSelectComponet" to "UserSelectComponent" in the describe block and related
test names (e.g., the describe('UserSelectComponet Structure') and
test('UserSelectComponet has all properties') occurrences) so the test suite and
messages correctly reference UserSelectComponent; update any other occurrences
of the same typo (such as the test at line 266) to ensure consistency.
In `@packages/structures/__tests__/message.embeds.test.ts`:
- Around line 243-256: The test's patch value uses "thumbnail" in the EmbedImage
test; update the patched proxy_url string in the kPatch call (the value
currently 'djs://[PATCHED]-embed-thumbnail-proxy-url') to a clearer
image-specific value such as 'djs://[PATCHED]-embed-image-proxy-url' so
assertions on patched.proxyURL and the in-place patch behavior on instance
(checked via patched === instance and patched.toJSON() !== data) remain
semantically correct.
- Line 118: The test's expectation for the hexColor getter is wrong because
hexColor pads values with padStart(6,'0'); update the assertion in
message.embeds.test.ts to compare against a zero-padded hex string by using
embedData.color!.toString(16).padStart(6,'0') (or alternatively change
embedData.color to a small value to explicitly assert padding), so the
expectation matches the Embed.hexColor getter behavior.
In `@packages/structures/__tests__/message.test.ts`:
- Around line 839-875: The describe block title is inconsistent with the class
under test; change the test suite name from 'ApplicationCommandInteraction
sub-structure' to reference the actual class
ApplicationCommandInteractionMetadata (e.g.,
'ApplicationCommandInteractionMetadata sub-structure') so the suite name matches
the tested symbol; update the string in the describe(...) that wraps tests for
the ApplicationCommandInteractionMetadata instance.
- Around line 703-710: The failing test is caused by optimizeData parsing hex
color strings with a leading '#' which makes Number.parseInt('#345324', 16)
return NaN; update the optimizeData implementation to strip the '#' prefix
before parsing (e.g., remove leading '#' from the color string prior to
Number.parseInt) so internal storage keeps the correct numeric value and toJSON
(method toJSON) can emit the original color string; after fixing optimizeData,
re-enable the commented toJSON() test in message.test.ts to verify correctness.
In `@packages/structures/__tests__/poll.test.ts`:
- Around line 45-46: Tests share a single Poll instance which is mutated across
cases; update the tests to create a fresh Poll for each test (either instantiate
inside each it(...) or use a beforeEach that sets a new const/let like instance
= new Poll(data)) so methods referencing instance (e.g., the Poll constructor
usage and later patch mutation tests) operate on a clean object and avoid
order-dependent failures; replace the top-level const instance = new Poll(data)
with per-test instantiation for the occurrences around lines that reference
instance in the patch tests.
In `@packages/structures/__tests__/subscription.test.ts`:
- Around line 48-55: The test currently mutates the module-scoped fixture by
assigning expectedData = data then deleting canceled_at; instead clone the
fixture into expectedData (e.g., a shallow copy) so deleting canceled_at only
affects expectedData, then assert
expect(instance.toJSON()).toStrictEqual(expectedData); refer to the variables
expectedData, data and the instance.toJSON() assertion when making the change.
- Around line 67-69: The test is currently parsing data.current_period_end for
the canceled timestamp assertion; update it to parse the patched canceled_at
value and assert against the patched value instead. Specifically, change the
assignment of canceledTimestamp to parse data.canceled_at (the patched value
used in the test) and use that patched timestamp when asserting
instance.canceledTimestamp and instance.canceledAt.valueOf(), so the test
actually validates the patched canceled_at field rather than current_period_end.
In `@packages/structures/src/skus/SKU.ts`:
- Around line 78-84: Rename the SKU getter createdDate to createdAt in the SKU
class so the API matches other structures: replace the public get createdDate()
{ const createdTimestamp = this.createdTimestamp; return createdTimestamp ? new
Date(createdTimestamp) : null; } with a public get createdAt() that uses the
same createdTimestamp-to-Date logic, update any internal references or usages of
createdDate to createdAt (including the test in
packages/structures/__tests__/sku.test.ts to assert instance.createdAt), and run
tests to ensure no remaining references to createdDate remain.
In `@packages/structures/src/voice/VoiceState.ts`:
- Around line 38-40: The request-to-speak cache is not cleared when a patch
explicitly sets request_to_speak_timestamp to null; update the patch handling in
VoiceState so it checks for the presence of the field (e.g., if
('request_to_speak_timestamp' in data)) and then if the value is null set
this[kRequestToSpeakTimestamp] = null, otherwise set
this[kRequestToSpeakTimestamp] = Date.parse(data.request_to_speak_timestamp);
ensure this change affects requestToSpeakTimestamp, requestToSpeakAt, and toJSON
by updating the single source kRequestToSpeakTimestamp inside the VoiceState
class.
- Line 37: The optimizeData override in VoiceState is declared public but should
remain protected to match the internal hook on Structure; change the method
signature of optimizeData in the VoiceState class from public to protected (keep
the override and method body intact) so it matches Structure.optimizeData's
visibility and does not expose the internal API surface; search for any external
callers of VoiceState.optimizeData and, if found, move them to use a public API
or keep calling from within class internals.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 681470a3-3fc7-489d-95b0-bdb745d13181
📒 Files selected for processing (22)
packages/structures/__tests__/automoderation.test.tspackages/structures/__tests__/channels.test.tspackages/structures/__tests__/emoji.test.tspackages/structures/__tests__/entitlement.test.tspackages/structures/__tests__/message.components.test.tspackages/structures/__tests__/message.embeds.test.tspackages/structures/__tests__/message.test.tspackages/structures/__tests__/poll.test.tspackages/structures/__tests__/sku.test.tspackages/structures/__tests__/soundboardsound.test.tspackages/structures/__tests__/stageinstance.test.tspackages/structures/__tests__/sticker.test.tspackages/structures/__tests__/subscription.test.tspackages/structures/__tests__/team.test.tspackages/structures/__tests__/user.test.tspackages/structures/__tests__/voice.test.tspackages/structures/__tests__/webhook.test.tspackages/structures/src/messages/components/ButtonComponent.tspackages/structures/src/messages/components/index.tspackages/structures/src/skus/SKU.tspackages/structures/src/utils/symbols.tspackages/structures/src/voice/VoiceState.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-16T18:45:40.408Z
Learnt from: Qjuh
Repo: discordjs/discord.js PR: 11423
File: packages/discord.js/src/structures/MessagePayload.js:193-206
Timestamp: 2026-02-16T18:45:40.408Z
Learning: In discord.js MessagePayload.js, when mapping files to attachments, custom ids set via AttachmentBuilder.setId() should override the index-based id. The spread operator should come after the explicit id assignment to allow toJSON() values to take precedence.
Applied to files:
packages/structures/__tests__/message.test.ts
🔇 Additional comments (13)
packages/structures/__tests__/poll.test.ts (1)
51-54: Good timestamp assertions for expiry getters.Parsing once and asserting both
expiresTimestampandexpiresAtagainst the same value is clean and robust.packages/structures/__tests__/message.embeds.test.ts (1)
106-110: Same shared mutable state pattern as other test files.The
embedsarray andinstanceare created once and shared across tests. The patch test at lines 125-136 mutates the instance, which could affect test isolation. This follows the same pattern flagged in entitlement.test.ts.packages/structures/src/messages/components/index.ts (1)
13-13: LGTM!The
LabelComponentexport is correctly added and maintains alphabetical ordering with the existing exports.packages/structures/src/messages/components/ButtonComponent.ts (1)
39-41: LGTM!The
disabledgetter now usesisFieldSetfor explicit type checking, returningnullwhen the field is absent or not a boolean. This aligns with the type-guard pattern established in the codebase.packages/structures/src/utils/symbols.ts (1)
17-18: LGTM!The new
kRequestToSpeakTimestampsymbol follows the established naming convention and is properly placed among other timestamp-related symbols.packages/structures/__tests__/message.test.ts (1)
476-538: Well-structured attachment sub-structure tests.Good coverage of attachment getters including the new
AttachmentFlagshandling. The test properly verifies both flagged and unflagged attachment instances.packages/structures/__tests__/channels.test.ts (1)
1-1: Good coverage addition for snowflake-derived creation time.The new assertions align with
TextChannel’s inheritedcreatedTimestamp/createdAtbehavior and strengthen regression coverage.Also applies to: 73-75
packages/structures/__tests__/sku.test.ts (1)
19-47: Comprehensive structure coverage looks solid.Getter assertions, snowflake-derived time checks,
toJSON()strictness, and in-placekPatchidentity checks provide good confidence for SKU behavior.packages/structures/__tests__/webhook.test.ts (1)
21-50: Webhook test suite is well-structured and complete for this scope.The tests verify public getters, snowflake-derived creation fields, strict serialization, and in-place patch behavior correctly.
packages/structures/__tests__/sticker.test.ts (1)
43-112: Nice end-to-end coverage for both Sticker and StickerPack.The suite validates getter mapping, snowflake timestamp/date derivation, strict serialization, and patch-in-place behavior effectively.
packages/structures/__tests__/emoji.test.ts (1)
29-55: Emoji tests provide solid behavior coverage.Good validation of getter parity, snowflake-derived creation fields, strict
toJSON()output, and in-place patch mutation.packages/structures/__tests__/stageinstance.test.ts (1)
20-50: StageInstance coverage looks good.Getter checks, snowflake timestamp/date assertions, strict serialization, and identity-preserving patch checks are all in place.
packages/structures/__tests__/user.test.ts (1)
55-165: Excellent breadth for User and nested sub-structures.This suite robustly validates getter mapping, snowflake-derived creation fields, serialization fidelity, and in-place patch semantics across
User,AvatarDecorationData, andConnection.
| // note guild_id is missing (to test kPatch) | ||
| }; | ||
|
|
||
| const instance = new Entitlement(data); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other test files also create shared instances outside test blocks that get mutated
rg -n 'const instance = new' packages/structures/__tests__/*.test.ts | head -30Repository: discordjs/discord.js
Length of output: 2794
🏁 Script executed:
cat -n packages/structures/__tests__/entitlement.test.tsRepository: discordjs/discord.js
Length of output: 2641
🏁 Script executed:
# Also check webhook.test.ts since it also has line 18 instance creation (appears top-level)
cat -n packages/structures/__tests__/webhook.test.ts | head -50Repository: discordjs/discord.js
Length of output: 2110
Shared mutable instance may cause test order dependency.
The instance is created once at line 21 (inside the describe block but outside test blocks) and mutated by the patching test (lines 54–57). While tests currently pass because toJSON() runs before patching, if test execution order changes or tests run in parallel, the toJSON() test could fail since instance state is modified. Consider creating a fresh instance within each test using beforeEach:
Proposed fix using beforeEach for test isolation
-const instance = new Entitlement(data);
+let instance: Entitlement;
+
+beforeEach(() => {
+ instance = new Entitlement(data);
+});This requires importing beforeEach from vitest:
-import { describe, expect, test } from 'vitest';
+import { beforeEach, describe, expect, test } from 'vitest';📝 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.
| const instance = new Entitlement(data); | |
| let instance: Entitlement; | |
| beforeEach(() => { | |
| instance = new Entitlement(data); | |
| }); |
| const instance = new Entitlement(data); | |
| import { beforeEach, describe, expect, test } from 'vitest'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/structures/__tests__/entitlement.test.ts` at line 21, The test
creates a shared Entitlement instance (const instance = new Entitlement(data))
at describe scope and one test mutates it, causing test-order dependency; fix by
moving creation into a per-test lifecycle (use beforeEach to re-instantiate the
Entitlement) so each test gets a fresh instance, and add the beforeEach import
from vitest; update references that currently use the top-level instance to use
the instance provided by beforeEach (e.g., the same variable name scoped to the
describe).
| if (data.request_to_speak_timestamp) { | ||
| this[kRequestToSpeakTimestamp] = Date.parse(data.request_to_speak_timestamp); | ||
| } |
There was a problem hiding this comment.
Clear the cached request-to-speak timestamp on explicit null patches.
Because request_to_speak_timestamp is omitted from kData, this cache is the only source of truth. With the current truthy check, a patch like { request_to_speak_timestamp: null } leaves the old value in place, so requestToSpeakTimestamp, requestToSpeakAt, and toJSON() all stay stale after the user stops requesting to speak.
Proposed fix
- if (data.request_to_speak_timestamp) {
- this[kRequestToSpeakTimestamp] = Date.parse(data.request_to_speak_timestamp);
+ if ('request_to_speak_timestamp' in data) {
+ this[kRequestToSpeakTimestamp] = data.request_to_speak_timestamp
+ ? Date.parse(data.request_to_speak_timestamp)
+ : null;
}📝 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 (data.request_to_speak_timestamp) { | |
| this[kRequestToSpeakTimestamp] = Date.parse(data.request_to_speak_timestamp); | |
| } | |
| if ('request_to_speak_timestamp' in data) { | |
| this[kRequestToSpeakTimestamp] = data.request_to_speak_timestamp | |
| ? Date.parse(data.request_to_speak_timestamp) | |
| : null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/structures/src/voice/VoiceState.ts` around lines 38 - 40, The
request-to-speak cache is not cleared when a patch explicitly sets
request_to_speak_timestamp to null; update the patch handling in VoiceState so
it checks for the presence of the field (e.g., if ('request_to_speak_timestamp'
in data)) and then if the value is null set this[kRequestToSpeakTimestamp] =
null, otherwise set this[kRequestToSpeakTimestamp] =
Date.parse(data.request_to_speak_timestamp); ensure this change affects
requestToSpeakTimestamp, requestToSpeakAt, and toJSON by updating the single
source kRequestToSpeakTimestamp inside the VoiceState class.
There was a problem hiding this comment.
Is there anywhere on the API docs where it says this?
There was a problem hiding this comment.
@AsadHumayun, while the Discord API documentation doesn't explicitly state "we send null to indicate a cancelled request-to-speak", there are two pieces of evidence that confirm null is a valid value for this field:
-
The
discord-api-typestype definition —APIVoiceState.request_to_speak_timestampis typed asstring | null, which means the API can and does sendnullfor this field. -
Real-world gateway payloads — an example voice state payload from the Discord API docs issue tracker shows
"request_to_speak_timestamp": null, confirming the gateway emitsnullin practice (e.g. for stage channel lurkers who aren't requesting to speak).
Additionally, when a user is unsuppressed, non-bot users will have their request_to_speak_timestamp set to the current time — and the inverse (suppressing/cancelling) would logically result in a null value being dispatched via a VOICE_STATE_UPDATE gateway event.
Since kRequestToSpeakTimestamp is the only source of truth for this field (it's stripped from kData), the suggested fix ensures a null patch correctly clears the cache rather than silently leaving a stale timestamp:
- if (data.request_to_speak_timestamp) {
- this[kRequestToSpeakTimestamp] = Date.parse(data.request_to_speak_timestamp);
+ if ('request_to_speak_timestamp' in data) {
+ this[kRequestToSpeakTimestamp] = data.request_to_speak_timestamp
+ ? Date.parse(data.request_to_speak_timestamp)
+ : null;
}
| */ | ||
| public get requestToSpeakTimestamp() { | ||
| return this[kData].request_to_speak_timestamp; | ||
| return this[kRequestToSpeakTimestamp] ? dateToDiscordISOTimestamp(new Date(this[kRequestToSpeakTimestamp])) : null; |
There was a problem hiding this comment.
Shouldn't this just be return this[kRequestToSpeakTimestamp]?
There was a problem hiding this comment.
Done, also @almeidx do you know if what CodeRabbit is on about is correct?
There was a problem hiding this comment.
Yes, since the kRequestToSpeakTimestamp is the "source of truth" and is only being set there.
If you look at the current discord.js, the common pattern for properties is to use in.
discord.js/packages/discord.js/src/structures/VoiceState.js
Lines 132 to 141 in 1f9affd
For the
null case here, null && ... -> null
3 participants
This PR adds tests for the following structures:
StickerPack)VoiceRegion,VoiceState)Note to test changes introduced in #11406 when it is eventually merged.