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.
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.
* chore: Set ColumnLimit to 120 in clang-format (XRPLF#6288) This change updates the ColumnLimit from 80 to 120, and applies clang-format to reformat the code. * chore: Remove unnecessary `boost::system` requirement from conanfile (XRPLF#6290) * chore: Add cmake-format pre-commit hook (XRPLF#6279) This change adds `cmake-format` as. a pre-commit hook. The style file closely matches that in Clio, and they will be made to be equivalent over time. For now, some files have been excluded, as those need some manual adjustments, which will be done in future changes. * chore: Format all cmake files without comments (XRPLF#6294) * ci: Update hashes of XRPLF/actions (XRPLF#6316) This updates the hashes of all XRPLF/actions to their latest versions. * chore: Add upper-case match for ARM64 in CompilationEnv (XRPLF#6315) * fix: Restore config changes that broke standalone mode (XRPLF#6301) When support was added for `xrpld.cfg` in addition to `rippled.cfg` in XRPLF#6098, as part of an effort to rename occurrences of ripple(d) to xrpl(d), the clearing and creation of the data directory were modified for what, at the time, seemed to result in an equivalent code flow. This has turned out to not be true, which is why this change restores two modifications to `Config.cpp` that currently break running the binary in standalone mode. * docs: Update API changelog, add APIv2+APIv3 version documentation (XRPLF#6308) This change cleans up the `API-CHANGELOG.md` file. It moves the version-specific documentation to other files and fleshes out the changelog with all the API-related changes in each version. * chore: Add .zed editor config directory to .gitignore (XRPLF#6317) This change adds the project configuration directory to `.gitignore` for the `zed` editor. As per the [documentation](https://zed.dev/docs/remote-development?highlight=.zed#zed-settings), the project configuration files are stored in the `.zed` directory at the project root dir. * fix: Deletes expired NFToken offers from ledger (XRPLF#5707) This change introduces the `fixExpiredNFTokenOfferRemoval` amendment that allows expired offers to pass through `preclaim()` and be deleted in `doApply()`, following the same pattern used for expired credentials. * refactor: Add ServiceRegistry to help modularization (XRPLF#6222) Currently we're passing the `Application` object around, whereby the `Application` class acts more like a service registry that gives other classes access to other services. In order to allow modularization, we should replace `Application` with a service registry class so that modules depending on `Application` for other services can be moved easily. This change adds the `ServiceRegistry` class. * chore: Remove unity builds (XRPLF#6300) Unity builds were intended to speed up builds, by bundling multiple files into compilation units. However, now that ccache is available on all platforms, there is no need for unity builds anymore, as ccache stores compiled individual build objects for reuse. This change therefore removes the ability to make unity builds. * refactor: Replace include guards by '#pragma once' (XRPLF#6322) This change replaces all include guards in the `src/` and `include/` directories by `#pragma once`. * chore: Remove unnecessary script (XRPLF#6326) * chore: Update secp256k1 and openssl (XRPLF#6327) * fix typo in LendingHelpers unit-test (XRPLF#6215) * fix: Increment sequence when accepting new manifests (XRPLF#6059) The `ManifestCache::applyManifest` function was returning early without incrementing `seq_`. `OverlayImpl `uses this sequence to identify/invalidate a cached `TMManifests` message, which is exchanged with peers on connection. Depending on network size, startup sequencing, and topology, this can cause syncing issues. This change therefore increments `seq_` when a new manifest is accepted. * refactor: Update secp256k1 to 0.7.1 (XRPLF#6331) The latest secp256k1 release, 0.7.1, contains bug fixes that we may benefit from, see https://github.com/bitcoin-core/secp256k1/blob/master/CHANGELOG.md. * chore: Restore unity builds (XRPLF#6328) In certain cases, such as when modifying headers used by many compilation units, performing a unity build is slower than when performing a regular build with `ccache` enabled. There is also a benefit to a unity build in that it can detect things such as macro redefinitions within the group of files that are compiled together as a unit. This change therefore restores the ability to perform unity builds. However, instead of running every configuration with and without unity enabled, it is now only enabled for a single configuration to maintain lower computational use. As part of restoring the code, it became clear that currently two configurations have coverage enabled, since the check doesn't focus specifically on Debian Bookworm so it also applies to Debian Trixie. This has been fixed too in this change. * perf: Remove unnecessary caches (XRPLF#5439) This change removes the cache in `DatabaseNodeImp` and simplifies the caching logic in `SHAMapStoreImp`. As NuDB and RocksDB internally already use caches, additional caches in the code are not very valuable or may even be unnecessary, as also confirmed during preliminary performance analyses. * chore: Remove CODEOWNERS (XRPLF#6337) * test: Add file and line location to Env (XRPLF#6276) This change uses `std::source_location` to output the file and line location of the call that triggered a failed transaction. * refactor: Fix spelling issues in tests (XRPLF#6199) 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. * refactor: Change main thread name to `xrpld-main` (XRPLF#6336) This change builds on the thread-renaming PR (XRPLF#6212), by renaming the main thread name to reduce ambiguity in performance monitoring tools. * fix: Update invariant checks for Permissioned Domains (XRPLF#6134) * refactor: Modularize WalletDB and Manifest (XRPLF#6223) This change modularizes the `WalletDB` and `Manifest`. Note that the wallet db has nothing to do with account wallets and it stores node configuration, which is why it depends on the manifest code. * refactor: Modularize RelationalDB (XRPLF#6224) The rdb module was not properly designed, which is fixed in this change. The module had three classes: 1) The abstract class `RelationalDB`. 2) The abstract class `SQLiteDatabase`, which inherited from `RelationalDB` and added some pure virtual methods. 3) The concrete class `SQLiteDatabaseImp`, which inherited from `SQLiteDatabase` and implemented all methods. The updated code simplifies this as follows: * The `SQLiteDatabaseImp` has become `SQLiteDatabase`, and * The former `SQLiteDatabase `has merged with `RelationalDatabase`. * chore: Fix `gcov` lib coverage build failure on macOS (XRPLF#6350) For coverage builds, we try to link against the `gcov` library (specific to the environment). But as macOS doesn't have this library and thus doesn't have the coverage tools to generate reports, the coverage builds on that platform were failing on linking. We actually don't need to explicitly force this linking, as the `CodeCoverage` file already has correct detection logic (currently on lines 177-193), which is invoked when the `--coverage` flag is provided: * AppleClang: Uses `xcrun -f llvm-cov` to set `GCOV_TOOL="llvm-cov gcov"`. * Clang: Finds `llvm-cov` to set `GCOV_TOOL="llvm-cov gcov"`. * GCC: Finds `gcov` to set `GCOV_TOOL="gcov"`. The `GCOV_TOOL` is then passed to `gcovr` on line 416, so the correct tool is used for processing coverage data. This change therefore removes the `gcov` suffix from lines 473 and 475 in the `CodeCoverage.cmake` file. * refactor: Modularize the NetworkOPs interface (XRPLF#6225) This change moves the NetworkOPs interface into `libxrpl` and it leaves its implementation in `xrpld`. * chore: Fix minor issues in comments (XRPLF#6346) * refactor: Modularize `HashRouter`, `Conditions`, and `OrderBookDB` (XRPLF#6226) This change modularizes additional components by moving code to `libxrpl`. * chore: Update clang-format to 21.1.8 (XRPLF#6352) * refactor: Decouple app/tx from `Application` and `Config` (XRPLF#6227) This change decouples app/tx from `Application` and `Config` to clear the way to moving transactors to `libxrpl`. * refactor: Modularize app/tx (XRPLF#6228) * ci: Add clang tidy workflow to ci (XRPLF#6369) * chore: Set cmake-format width to 100 (XRPLF#6386) * chore: Set clang-format width to 100 in config file (XRPLF#6387) * chore: Apply clang-format width 100 (XRPLF#6387) * Fix tautological assertion (XRPLF#6393) * ci: Add dependabot config (XRPLF#6379) * ci: Build docs in PRs and in private repos (XRPLF#6400) * ci: [DEPENDABOT] bump codecov/codecov-action from 5.4.3 to 5.5.2 (XRPLF#6398) Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 5.4.3 to 5.5.2. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@18283e0...671740a) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-version: 5.5.2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Fix merge conflicts, format, spelling, replace all include guards by #pragma once * Fix non-unity build --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Ayaz Salikhov <mathbunnyru@users.noreply.github.com> Co-authored-by: Ed Hennis <ed@ripple.com> Co-authored-by: Bart <bthomee@users.noreply.github.com> Co-authored-by: Mayukha Vadari <mvadari@ripple.com> Co-authored-by: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Jingchen <a1q123456@users.noreply.github.com> Co-authored-by: Niq Dudfield <ndudfield@gmail.com> Co-authored-by: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> Co-authored-by: Olek <115580134+oleks-rip@users.noreply.github.com> Co-authored-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Co-authored-by: nuxtreact <nuxtreact@outlook.com> Co-authored-by: Sergey Kuznetsov <skuznetsov@ripple.com> Co-authored-by: Ayaz Salikhov <asalikhov@ripple.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.