refactor: fix spelling issues in tests - #6199
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables spell checking for test files by removing the src/test/** exception from the cspell configuration and systematically fixes all spelling issues found in test code. The changes include improved variable naming conventions, better function names, grammar fixes in comments, and more realistic test data strings.
Key Changes:
- Updated cspell configuration to enable spell checking on test files with appropriate string literal ignores
- Renamed functions to use proper camelCase (e.g.,
testCTIDRPC→testCtidRPC,createjv→createJV) - Improved variable names for clarity and removed possessives (e.g.,
bobsOffer→bobOffer,enableds→enabledAmendments)
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| .config/cspell.config.yaml | Removed test directory from ignore list, added test file string literal ignores, added CTID regex pattern, and new dictionary words |
| src/test/rpc/Transaction_test.cpp | Renamed testCTIDRPC to testCtidRPC for proper camelCase |
| src/test/rpc/NoRipple_test.cpp | Renamed gline0 to gwLine0 for clarity |
| src/test/rpc/LedgerEntry_test.cpp | Renamed alicesAcctRootBinary to aliceAcctRootBinary (removed possessive) |
| src/test/rpc/Handler_test.cpp | Added cspell comment to ignore "stdev" |
| src/test/rpc/AccountObjects_test.cpp | Renamed bobs_account_objects to bob_account_objects (removed possessive) |
| src/test/rpc/AccountLines_test.cpp | Fixed comment grammar from "beckys" to "becky's" and "alices" to "alice's" |
| src/test/protocol/STAmount_test.cpp | Renamed smallXsmall to smallXSmall for proper camelCase |
| src/test/overlay/compression_test.cpp | Improved variable names from jrequestUsd/jreply_usd to requestUsd/replyUSD |
| src/test/ledger/View_test.cpp | Renamed enableds to enabledAmendments for clarity |
| src/test/ledger/Directory_test.cpp | Fixed comment grammar from "Alices creates" to "Alice creates" |
| src/test/jtx/mpt.h | Renamed functions to proper camelCase: createjv → createJV, destroyjv → destroyJV, etc. |
| src/test/jtx/impl/mpt.cpp | Updated function implementations and error messages to match header changes |
| src/test/jtx/impl/TestHelpers.cpp | Renamed allpe to allPathElements for clarity |
| src/test/jtx/impl/AMM.cpp | Renamed jvflags to jvFlags and jvres to jvRes for proper camelCase |
| src/test/jtx/TestHelpers.h | Updated function declaration to match implementation |
| src/test/core/Config_test.cpp | Updated test data strings to use hyphens in domain names for realism |
| src/test/app/XChain_test.cpp | Added cspell comments to ignore "NonBatch" in function name and "attns" abbreviation |
| src/test/app/TrustAndBalance_test.cpp | Renamed invoiceid to invoiceId for proper camelCase |
| src/test/app/SHAMapStore_test.cpp | Renamed oinfo to outInfo for clarity |
| src/test/app/ReducedOffer_test.cpp | Removed possessives from variable names: bobsFee → bobFee, bobsOffer → bobOffer, etc. |
| src/test/app/PayStrand_test.cpp | Renamed xrpsi to xrpStepInfo and updated function call from allpe to allPathElements |
| src/test/app/Offer_test.cpp | Removed possessives from variable names throughout |
| src/test/app/NFToken_test.cpp | Renamed nftOnlyXRPID to nftOnlyXrpID and similar variables for consistency |
| src/test/app/MPToken_test.cpp | Fixed comment grammar from "bobs'" to "bob's" |
| src/test/app/Loan_test.cpp | Added cspell comment to ignore "LOANTODO" |
| src/test/app/LoanBroker_test.cpp | Fixed spelling in comment from "AllowTrustLineClaback" to "AllowTrustLineClawback" |
| src/test/app/LPTokenTransfer_test.cpp | Fixed comment grammar from "bobs's" to "bob's" |
| src/test/app/FixNFTokenPageLinks_test.cpp | Fixed comment grammar from "alices's" to "alice's" |
| src/test/app/EscrowToken_test.cpp | Fixed comment grammar and renamed testIOUINSF to testIOUInsufficientFunds |
| src/test/app/DepositAuth_test.cpp | Renamed readedCreds to readCreds for proper past tense |
| src/test/app/CrossingLimits_test.cpp | Renamed bobsOfferCount to bobOfferCount and evitasOfferCount to evitaOfferCount |
| src/test/app/Batch_test.cpp | Fixed comment grammar from "bobs" to "bob's" |
| src/test/app/AMM_test.cpp | Fixed spelling of "nataly" to "natalie" and improved comment grammar |
| src/test/app/AMMExtended_test.cpp | Renamed AMMXRPPool to AmmXrpPool and updated function call |
| src/test/app/AMMCalc_test.cpp | Renamed type trates to transfer_rates, variables for clarity, and fixed comment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6199 +/- ##
=========================================
- Coverage 79.9% 79.9% -0.0%
=========================================
Files 840 840
Lines 65483 65483
Branches 7251 7255 +4
=========================================
- Hits 52319 52311 -8
- Misses 13164 13172 +8 see 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tsCOMMITED = 3; // in a closed ledger | ||
| tsNEW = 1; // origin node did/could not validate | ||
| tsCURRENT = 2; // scheduled to go in this ledger | ||
| tsCOMMITTED = 3; // in a closed ledger |
There was a problem hiding this comment.
This will be a breaking change for those using the compiled proto. However, the number on the wire stays the same, which is a positive. It would be an easy fix for anyone using this proto whose code will fail to compile, but it would be a surprise if not properly communicated.
A way to more gracefully fix this:
- Add
option allow_alias = true;to the enum definition. - Add
[deprecated = true];and the cspell ignore to thetsCOMMITEDentry. - Add
tsCOMMITTED = 3;below the deprecated value.
Then in 6 months we can remove the deprecated value and remove the alias stanza, essentially achieving what you have right now.
There was a problem hiding this comment.
Why is this xrpl.proto change a problem but all the ones in #5719 not?
There was a problem hiding this comment.
It's the same problem there. I missed that they were proto enums and not C++ enums. If @godexsoft is ok with the proto changes and it is unlikely to severely affect downstream consumers (maybe it's only Clio?) then we can go ahead. Otherwise it would be wise to add aliases to all modified enum values.
There was a problem hiding this comment.
Clio only uses the org/xrpl/v1/*.proto so i don't think this would break anything for us. I'm not aware of any other consumers of proto and their use cases though.
| suggestWords: | ||
| - xprl->xrpl | ||
| - unsynched->unsynced | ||
| - unsynched->unsynced # cspell: disable-line not sure what this problem is.... |
There was a problem hiding this comment.
CSpell checks its own config file and trips over it, is that what happens here?
There was a problem hiding this comment.
I guess? I was confused as to why only this one was flagged though
Co-authored-by: Bart <bthomee@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 54 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tsCOMMITED = 3; // in a closed ledger | ||
| tsNEW = 1; // origin node did/could not validate | ||
| tsCURRENT = 2; // scheduled to go in this ledger | ||
| tsCOMMITTED = 3; // in a closed ledger |
There was a problem hiding this comment.
The spelling correction from "tsCOMMITED" to "tsCOMMITTED" changes the enum value name in a protobuf file. This is a breaking API change that will affect any code that uses this enum value. The enum value name is part of the external API and changing it will break compatibility with existing code that references "tsCOMMITED".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
cspell.config.yaml:36
- The new override for
**/*_test.cppignores all quoted strings (single, double, and backtick). That effectively disables spellchecking for JSON blobs, error messages, and other string literals in tests, which significantly reduces the value of removingsrc/test/**fromignorePaths. Consider narrowing these ignore patterns to the specific kinds of strings that are noisy (e.g., long hex/base58/base64 tokens), rather than ignoring every quoted string.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| testRangeRequest(features); | ||
| testRangeCTIDRequest(features); | ||
| testCTIDValidation(features); | ||
| testCTIDRPC(features); | ||
| testRPCsforCTID(features); | ||
| forAllApiVersions(std::bind_front(&Transaction_test::testRequest, this, features)); |
There was a problem hiding this comment.
testWithFeats() calls testRPCsforCTID(features), but the method defined in this class is testRPCforCTID(FeatureBitset). There is no testRPCsforCTID symbol in the file, so this will fail to compile. Rename the call to match the existing method (or rename the method and update all call sites consistently).
| enum TransactionStatus { | ||
| tsNEW = 1; // origin node did/could not validate | ||
| tsCURRENT = 2; // scheduled to go in this ledger | ||
| tsCOMMITED = 3; // in a closed ledger | ||
| tsNEW = 1; // origin node did/could not validate | ||
| tsCURRENT = 2; // scheduled to go in this ledger | ||
| tsCOMMITTED = 3; // in a closed ledger | ||
| tsREJECT_CONFLICT = 4; |
There was a problem hiding this comment.
Renaming the protobuf enum value from tsCOMMITED to tsCOMMITTED changes the generated API for any consumers of xrpl.proto (even though the numeric value stays the same). This contradicts the PR description's "No functionality/test change" and "API Impact: N/A". If you need the spelling fix without breaking downstream code, consider keeping the old name as a deprecated alias (requires enabling enum allow_alias) or updating the PR description to reflect the breaking API change.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mvadari
added
Ready to merge
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
cspell.config.yaml:36
- The new
overridesentry ignores all double-quoted, single-quoted, and backtick-quoted strings in**/*_test.cpp. This effectively disables spellchecking for human-readable test names/descriptions liketestcase("..."), which undermines the goal of enabling cspell on tests. Consider narrowing these ignore patterns to only match known-noisy cases (e.g., long hex/base58 blobs or JSON fixtures) instead of all quoted strings.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cspell.config.yaml:36
- The new cspell override for
**/*_test.cppignores all quoted strings. This effectively configures away a large class of spelling errors in tests (including asserted/user-visible messages), which seems at odds with the PR description of “fixes all the issues that arise” after removing the test-path exception. Consider narrowing this to specific patterns (e.g., hex/base58, raw JSON blobs) or documenting in the PR/config why skipping all string literals in tests is desired.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cspell.config.yaml:27
- The GitHub username ignore regex appears to have an unescaped closing
)while the opening(is escaped (\(). In JavaScript-style regexes (which cspell uses), this is typically an "unmatched ')'" syntax error and can break the spellcheck run. Escape the closing parenthesis (\)) or adjust the pattern to avoid a stray).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This change removes the `src/tests` exception from the `cspell` config and fixes all the issues that arise as a result. No functionality/test change.
High Level Overview of Change
This PR removes the
src/testsexception from the cspell config and fixes all the issues that arise as a result. No functionality/test change.Context of Change
More cspell work, follow-up from #5719
Type of Change
API Impact
N/A
Test Plan
CI passes.