Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Match unit tests on start of test name, and split some test classes #4634

Merged
merged 6 commits into from
Sep 14, 2023

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Jul 25, 2023

High Level Overview of Change

Changes the matching of the parameters to the --unittest command line option to match the patterns against the beginning of the test name.

  • For example, without this change, to run the TxQ tests, one must specify --unittest=TxQ1,TxQ2 on the command line. With this change, one can use --unittest=TxQ, and both will be run.
  • An exact match will prevent any further partial matching.
  • This could have some side effects for different tests with a common start of name. For example, NFToken, NFTokenBurn, NFTokenDir. This might be useful. If not, the shorter-named test(s) can be renamed. For example, NFToken to NFTokenBase.

Speaking of NFToken, the second commit splits it, along with the NFTokenBurn, and Offer test classes into 4 or 5 sub-classes each, based on the different FeatureBitsets that are currently run sequentially. In Windows, this speeds up a Debug build unit test run by about 5x! (Details below.)

There are two commits. The don't necessarily need to be squashed when merging, though it would be fine if they are.

Context of Change

Long test run times have been a thorn in all of our sides for a while, but are even worse in Windows Debug builds, because there is a ton of overhead. At the recent team summit, someone threw out the idea of adding regex parsing to the matching. This doesn't do full regex matching, but that keeps it pretty simple.

Note that these changes only affect unit tests.

Type of Change

  • [X ] New feature (non-breaking change which adds functionality)
  • [X ] Tests (You added tests for code that already exists, or your new feature included in this PR)

Before / After

Before:

These examples are with the tip of develop, commit aded4a7 as of this writing

--unittest=TxQ (fails)

$ rippled --unittest=TxQ
Failed: No tests run
119ms, 0 suites, 0 cases, 0 tests total, 1 failure

--unittest=TxQ1, TxQ2 (works)

$ rippled --unittest=TxQ1,TxQ2
ripple.app.TxQ1 queue sequence
[...]
ripple.app.TxQ2 acct in queue but empty
[...]
Longest suite times:
   24.4s ripple.app.TxQ2
   21.4s ripple.app.TxQ1
45.8s, 2 suites, 37 cases, 4787 tests total, 0 failures

Full test run with multiple runners --unittest --unittest-jobs=16 (2228.9 seconds, bound by the longest running test)

$ rippled --unittest --unittest-jobs=16
[...]
Longest suite times:
 2227.4s ripple.tx.NFToken
 1192.5s ripple.tx.NFTokenBurn
  288.9s ripple.tx.Offer
  259.0s ripple.app.TheoreticalQuality
  224.8s ripple.app.ShardArchiveHandler
  224.1s ripple.tx.NFTokenDir
  199.7s ripple.tx.ReducedOffer
  196.5s ripple.app.AMMExtended
  166.9s ripple.app.AMM
  163.4s ripple.app.ReportingETL
2228.9s, 211 suites, 1792 cases, 633774 tests total, 0 failures

After:

--unittest=TxQ (succeeds!)

$ rippled --unittest=TxQ
ripple.app.TxQ1 queue sequence
[...]
ripple.app.TxQ2 acct in queue but empty
[...]
Longest suite times:
   25.2s ripple.app.TxQ2
   22.1s ripple.app.TxQ1
47.4s, 2 suites, 37 cases, 4787 tests total, 0 failures

--unittest=TxQ1, TxQ2 (results unchanged)

$  ./build/cmake/msvc19.ON//Debug/rippled --unittest=TxQ1,TxQ2
ripple.app.TxQ1 queue sequence
[...]
ripple.app.TxQ2 acct in queue but empty
[...]
Longest suite times:
   25.4s ripple.app.TxQ2
   22.1s ripple.app.TxQ1
47.6s, 2 suites, 37 cases, 4787 tests total, 0 failures

Full test run with multiple runners --unittest --unittest-jobs=16 (438.7 seconds, not bound by the longest running test)

$ rippled --unittest --unittest-jobs=16
[...]
Longest suite times:
  314.6s ripple.app.TheoreticalQuality
  293.4s ripple.tx.NFToken0
  281.7s ripple.tx.NFToken4
  279.3s ripple.tx.NFToken1
  273.5s ripple.tx.NFToken2
  271.7s ripple.tx.NFToken3
  261.2s ripple.app.ShardArchiveHandler
  220.9s ripple.tx.NFTokenDir
  216.4s ripple.tx.ReducedOffer
  215.8s ripple.tx.NFTokenBurn4
438.7s, 222 suites, 1806 cases, 633788 tests total, 0 failures

The fact that the overall test run takes longer than the longest suite (TheoreticalQuality in this case) means that that longest running test suite finished before all the other tests were finished, which allowed the child to be reused to run more tests. I haven't seen that happen in on a full run in any configuration in years.

Test Plan

Because only unit test code has changed, as long as exiting tests pass as they normally do, there's no additional functional testing needed.

Future Tasks

Full regex parsing?

ximinez added 2 commits July 25, 2023 16:19
* For example, without this change, to run the TxQ tests, must specify
  `--unittest=TxQ1,TxQ2` on the command line. With this change, can use
  `--unittest=TxQ`, and both will be run.
* An exact match will prevent any further partial matching.
* This could have some side effects for different tests with a common
  name beginning. For example, NFToken, NFTokenBurn, NFTokenDir. This
  might be useful. If not, the shorter-named test(s) can be renamed. For
  example, NFToken to NFTokenBase.
* Potentially speeds up parallel tests by a factor of 5
* upstream/develop:
  Set version to 1.12.0-rc1
  Set version to 1.12-b3
  Several changes to improve Consensus stability: (4505)
  Apply transaction batches in periodic intervals (4504)
  Asynchronously write batches to NuDB (4503)
  refactor: fix typo in FeeUnits.h (4644)
  Update Ubuntu build image (4650)
  test: add forAllApiVersions helper function (4611)
  add view updates for account SLEs (4629)
  Fix the package recipe for consumers of libxrpl (4631)
  refactor: improve checking of path lengths (4519)
  refactor: use C++20 function std::popcount (4389)
  fix(AMM): prevent orphaned objects, inconsistent ledger state: (4626)
  feat: support Concise Transaction Identifier (CTID) (XLS-37) (4418)
@intelliot intelliot requested review from HowardHinnant and removed request for scottschurr August 24, 2023 16:35
Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

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

Nice!

@intelliot intelliot added this to the 1.13 milestone Aug 29, 2023
@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Aug 29, 2023
* upstream/develop:
  Set version to 1.12.0-rc3
  Revert "Asynchronously write batches to NuDB (4503)"
  Revert "Apply transaction batches in periodic intervals (4504)"
  Revert "Several changes to improve Consensus stability: (4505)"
@intelliot
Copy link
Collaborator

I guess ximinez:unittest-prefix has a branch protection rule requiring linear history? Anyway, GitHub can't automatically update this branch.

@ximinez , at your convenience, feel free to bring this up-to-date with develop. Also, if you'd like the commit message to differ from what you've already used in the commits here, feel free to provide a suggested commit message as well.

* upstream/develop:
  docs: update SECURITY.md (4338)
  refactor: simplify `TxFormats` common fields logic (4637)
  APIv2(ledger_entry): return invalidParams for bad parameters (4630)
  docs(rippled-example.cfg): clarify ssl_cert vs ssl_chain (4667)
  Introduce replacement for getting and setting thread name: (4312)
  Set version to 1.12.0
  Set version to 1.12.0-rc4
  amm_info: fetch by amm account id; add AMM object entry (4682)
  AMMBid: use tecINTERNAL for 'impossible' errors (4674)
@ximinez
Copy link
Collaborator Author

ximinez commented Sep 11, 2023

I guess ximinez:unittest-prefix has a branch protection rule requiring linear history? Anyway, GitHub can't automatically update this branch.

Turns out I did. Totally forgot about that. I turned it off.

at your convenience, feel free to bring this up-to-date with develop. Also, if you'd like the commit message to differ from what you've already used in the commits here, feel free to provide a suggested commit message as well.

Merge is done. Assuming this gets squashed down to one commit, I think the following would be a fine commit message:

Match unit tests on start of test name

* For example, without this change, to run the TxQ tests, must specify
  `--unittest=TxQ1,TxQ2` on the command line. With this change, can use
  `--unittest=TxQ`, and both will be run.
* An exact match will prevent any further partial matching.
* This could have some side effects for different tests with a common
  name beginning. For example, NFToken, NFTokenBurn, NFTokenDir. This
  might be useful. If not, the shorter-named test(s) can be renamed. For
  example, NFToken to NFTokens.
* Split the NFToken, NFTokenBurn, and Offer test classes. Potentially speeds
  up parallel tests by a factor of 5.

* upstream/develop:
  Several changes to improve Consensus stability: (4505)
  Apply transaction batches in periodic intervals (4504)
  Asynchronously write batches to NuDB. (4503)
  Remove CurrentThreadName.h from RippledCore.cmake (4697)
@intelliot
Copy link
Collaborator

@manojsdoshi - are we ok to merge this to develop now?

@intelliot intelliot merged commit 5427321 into XRPLF:develop Sep 14, 2023
@intelliot intelliot mentioned this pull request Sep 14, 2023
1 task
@ximinez ximinez deleted the unittest-prefix branch September 14, 2023 22:41
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
* For example, without this change, to run the TxQ tests, must specify
  `--unittest=TxQ1,TxQ2` on the command line. With this change, can use
  `--unittest=TxQ`, and both will be run.
* An exact match will prevent any further partial matching.
* This could have some side effects for different tests with a common
  name beginning. For example, NFToken, NFTokenBurn, NFTokenDir. This
  might be useful. If not, the shorter-named test(s) can be renamed. For
  example, NFToken to NFTokens.
* Split the NFToken, NFTokenBurn, and Offer test classes. Potentially speeds
  up parallel tests by a factor of 5.
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
* For example, without this change, to run the TxQ tests, must specify
  `--unittest=TxQ1,TxQ2` on the command line. With this change, can use
  `--unittest=TxQ`, and both will be run.
* An exact match will prevent any further partial matching.
* This could have some side effects for different tests with a common
  name beginning. For example, NFToken, NFTokenBurn, NFTokenDir. This
  might be useful. If not, the shorter-named test(s) can be renamed. For
  example, NFToken to NFTokens.
* Split the NFToken, NFTokenBurn, and Offer test classes. Potentially speeds
  up parallel tests by a factor of 5.
intelliot pushed a commit that referenced this pull request Jan 20, 2024
Update the "rippled --help" message for the "-u" parameter. This
documents the unit test name pattern matching rule implemented by #4634.

Fix #4800
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
* For example, without this change, to run the TxQ tests, must specify
  `--unittest=TxQ1,TxQ2` on the command line. With this change, can use
  `--unittest=TxQ`, and both will be run.
* An exact match will prevent any further partial matching.
* This could have some side effects for different tests with a common
  name beginning. For example, NFToken, NFTokenBurn, NFTokenDir. This
  might be useful. If not, the shorter-named test(s) can be renamed. For
  example, NFToken to NFTokens.
* Split the NFToken, NFTokenBurn, and Offer test classes. Potentially speeds
  up parallel tests by a factor of 5.
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
…LF#4846)

Update the "rippled --help" message for the "-u" parameter. This
documents the unit test name pattern matching rule implemented by XRPLF#4634.

Fix XRPLF#4800
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Will Need Documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants