Skip to content

Reduce macro usage in tests #2028

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

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

In total, this reduces the --profile=test --lib Zpretty=expanded code size from 334,247 LoC to 290,689 LoC.

@TheBlueMatt
Copy link
Collaborator Author

To be clear, the line counts here are rustfmt on the entire crate being shoved into one file, so not only is it the usual rustfmt vertical code, but it's got three extra indentations on the front so everything is split across tons of lines.

@TheBlueMatt TheBlueMatt force-pushed the 2023-02-macros-for-wilmer branch from 0558c31 to 12136e8 Compare February 15, 2023 19:18
@dunxen
Copy link
Contributor

dunxen commented Feb 21, 2023

Just looks like the type safety is getting us now with the test Node vs NodeHolder.
Also ambiguous docs error. Then looks good for squash after.

@TheBlueMatt TheBlueMatt force-pushed the 2023-02-macros-for-wilmer branch from 12136e8 to d215969 Compare February 21, 2023 19:54
@TheBlueMatt
Copy link
Collaborator Author

Oops, right, thanks.

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Basically LGTM. CI needs a fix. I didn't get a reduction in cargo test -p lightning time compared to main but also didn't measure very scientifically.

/// Gets an RAA and CS which were sent in response to a commitment update
///
/// Should only be used directly when the `$node` is not actually a [`Node`].
macro_rules! do_get_revoke_commit_msgs {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this commit still make sense? It's now a macro that calls a function that calls a macro. Is the LoC reduced documented in the commit message still accurate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didnt rerun the commit message, but it should be? The outer macro gets called all over the place, which compiles down to one function. That function body happens to be implemented with a macro, but its still only done once.

@TheBlueMatt TheBlueMatt force-pushed the 2023-02-macros-for-wilmer branch from d215969 to 6def325 Compare February 24, 2023 00:29
@TheBlueMatt
Copy link
Collaborator Author

I'll rerun the tests and correct the LoC counters in the commit messages before we land.

@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2023

Codecov Report

Patch coverage: 95.34% and project coverage change: +0.12 🎉

Comparison is base (8311581) 87.21% compared to head (761012a) 87.33%.

❗ Current head 761012a differs from pull request most recent head 0512260. Consider uploading reports for the commit 0512260 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2028      +/-   ##
==========================================
+ Coverage   87.21%   87.33%   +0.12%     
==========================================
  Files         100      101       +1     
  Lines       44516    44440      -76     
  Branches    44516    44440      -76     
==========================================
- Hits        38825    38813      -12     
+ Misses       5691     5627      -64     
Impacted Files Coverage Δ
lightning/src/ln/functional_test_utils.rs 88.75% <94.93%> (+0.32%) ⬆️
lightning/src/ln/channelmanager.rs 86.54% <100.00%> (-0.05%) ⬇️
lightning/src/ln/priv_short_conf_tests.rs 95.70% <100.00%> (ø)
lightning/src/sync/mod.rs 50.00% <0.00%> (-16.67%) ⬇️
lightning/src/util/time.rs 40.00% <0.00%> (-2.86%) ⬇️
lightning/src/ln/outbound_payment.rs 79.13% <0.00%> (-1.48%) ⬇️
lightning/src/sync/nostd_sync.rs 42.50% <0.00%> (-1.41%) ⬇️
lightning/src/util/events.rs 25.36% <0.00%> (-0.73%) ⬇️
lightning/src/ln/features.rs 92.55% <0.00%> (-0.33%) ⬇️
... and 32 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@valentinewallace
Copy link
Contributor

CI is sad, but otherwise LGTM

@TheBlueMatt TheBlueMatt force-pushed the 2023-02-macros-for-wilmer branch 3 times, most recently from ed41b1b to 3675b3d Compare February 24, 2023 20:56
@TheBlueMatt
Copy link
Collaborator Author

CI failures were a silent rebase conflict. I went ahead and rebased and squashed the fixups. Also went ahead and updated the line counts in the commits.

@TheBlueMatt
Copy link
Collaborator Author

With the changes in #1497, this takes my cargo test asdfasdfasdfasdf (ie build tests but dont run time) time when making a trivial changes to functional_tests.rs from 17.8 seconds to 12.4 seconds.

@valentinewallace
Copy link
Contributor

Needs rebase :/

The `check_added_monitors!()` macro has no reason to be a macro so
here we move its logic to a function and leave the macro in place
to avoid touching every line of code in the tests.

This reduces the `--profile=test --lib` `Zpretty=expanded` code
size from 338,710 LoC to 329,119 LoC.
The `get_payment_preimage_hash!()` macro has no reason to be a
macro so here we move its logic to a function and leave the macro
in place to avoid touching every line of code in the tests.

This reduces the `--profile=test --lib` `Zpretty=expanded` code
size from 329,119 LoC to 326,588 LoC.
The `get_route!()` macro has no reason to be a macro so here we
move its logic to a function and leave the macro in place to
avoid touching every line of code in the tests.

This reduces the `--profile=test --lib` `Zpretty=expanded` code
size from 326,588 LoC to 324,763 LoC.
The `get_revoke_commit_msgs!()` macro has no reason to be a macro
so here we move its logic to a function and leave the macro in
place to avoid touching every line of code in the tests.

This reduces the `--profile=test --lib` `Zpretty=expanded` code
size from 324,763 LoC to 322,183 LoC.
The `get_err_msg!()` macro has no reason to be a macro so here we
move its logic to a function and leave the macro in place to avoid
touching every line of code in the tests.

This reduces the `--profile=test --lib` `Zpretty=expanded` code
size from 322,183 LoC to 321,985 LoC.
The `get_htlc_update_msgs!()` macro has no reason to be a macro
so here we move its logic to a function and leave the macro in
place to avoid touching every line of code in the tests.

This reduces the `--profile=test --lib` `Zpretty=expanded` code
size from 321,985 LoC to 316,856 LoC.
While we cannot move the entire `check_spends` macro into a
function, we can move parts of it out, which we do here.

This reduces the `--profile=test --lib` `Zpretty=expanded` code
size from 316,856 LoC to 313,312 LoC.
The `check_closed_broadcast!()` macro has no reason to be a macro
so here we move its logic to a function and leave the macro in
place to avoid touching every line of code in the tests.

This reduces the `--profile=test --lib` `Zpretty=expanded` code
size from 313,312 LoC to 309,522 LoC.
The `check_closed_event!()` macro has no reason to be a macro so
here we move its logic to a function and leave the macro in place
to avoid touching every line of code in the tests.

This reduces the `--profile=test --lib` `Zpretty=expanded` code
size from 309,522 LoC to 301,915 LoC.
The `expect_pending_htlcs_forwardable*` macros don't need to be
macros so here we move much of the logic in them to a function and
leave the macro in place to avoid touching every line of code in
the tests.

This reduces the `--profile=test --lib` `Zpretty=expanded` code
size from 301,915 LoC to 295,294 LoC.
@TheBlueMatt
Copy link
Collaborator Author

Rebased again.

@TheBlueMatt TheBlueMatt merged commit c5cc1ed into lightningdevkit:main Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants