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

Move router test utils into their own module #1731

Conversation

valentinewallace
Copy link
Contributor

Used in #1724 to test onion message pathfinding

Useful for testing onion message pathfinding in upcoming PRs and any future
routing modules we add
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Generally looks good, but it could get a bit confusing that there are now two test_utils that are even both referenced in router.rs. Maybe we could solve this for the time being by removing the use statements and always using the full routing::test_utils/util::test_utils prefix to make it explicit?

To disambiguate it from routing::test_utils
@valentinewallace
Copy link
Contributor Author

Generally looks good, but it could get a bit confusing that there are now two test_utils that are even both referenced in router.rs. Maybe we could solve this for the time being by removing the use statements and always using the full routing::test_utils/util::test_utils prefix to make it explicit?

Hmm I feel like that would add a lot of verbosity since both of those test util modules are used in a ton of places.

Do you think it would help if I modified the import to be: use util::test_utils as ln_test_utils; so that all the current e.g. test_utils::TestScorer would be ln_test_utils::TestScorer?

TheBlueMatt
TheBlueMatt previously approved these changes Sep 20, 2022
@TheBlueMatt
Copy link
Collaborator

I'm fine with any resolution to the module resolution.

dunxen
dunxen previously approved these changes Sep 20, 2022
Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

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

Makes sense and LGTM!

@tnull
Copy link
Contributor

tnull commented Sep 21, 2022

Do you think it would help if I modified the import to be: use util::test_utils as ln_test_utils; so that all the current e.g. test_utils::TestScorer would be ln_test_utils::TestScorer?

Yup, use util::test_utils as ln_test_utils; would def. do the trick. No hard requirement though, feel free to leave it out if you think it gets too verbose, moving some to bench_utils already helped.

Also, feel free to squash.

@valentinewallace valentinewallace dismissed stale reviews from dunxen and TheBlueMatt via 2f19b64 September 21, 2022 16:26
@valentinewallace
Copy link
Contributor Author

Yup, use util::test_utils as ln_test_utils; would def. do the trick. No hard requirement though, feel free to leave it out if you think it gets too verbose, moving some to bench_utils already helped.

Also, feel free to squash.

Sg, I went ahead with that. I kinda prefer to keep the first big commit as a "pure code move" (modulo imports/exposing stuff as pub(super)) so I kept the commits separate

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2022

Codecov Report

Base: 90.70% // Head: 90.82% // Increases project coverage by +0.12% 🎉

Coverage data is based on head (2f19b64) compared to base (d698898).
Patch coverage: 97.08% of modified lines in pull request are covered.

❗ Current head 2f19b64 differs from pull request most recent head 95a4173. Consider uploading reports for the commit 95a4173 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1731      +/-   ##
==========================================
+ Coverage   90.70%   90.82%   +0.12%     
==========================================
  Files          86       87       +1     
  Lines       46679    48356    +1677     
  Branches    46679    48356    +1677     
==========================================
+ Hits        42338    43920    +1582     
- Misses       4341     4436      +95     
Impacted Files Coverage Δ
lightning/src/routing/gossip.rs 91.44% <ø> (ø)
lightning/src/routing/router.rs 91.62% <95.00%> (-0.45%) ⬇️
lightning/src/routing/test_utils.rs 97.93% <97.93%> (ø)
lightning/src/ln/functional_tests.rs 96.88% <0.00%> (-0.11%) ⬇️
lightning/src/chain/onchaintx.rs 95.63% <0.00%> (+1.14%) ⬆️
lightning/src/ln/channelmanager.rs 87.85% <0.00%> (+2.82%) ⬆️

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.

To disambiguate between util::test_utils and router::test_utils
@valentinewallace valentinewallace force-pushed the 2022-09-router-test-utils-mod branch from 2f19b64 to 95a4173 Compare September 21, 2022 19:10
@valentinewallace
Copy link
Contributor Author

I don't think that CI failure is related to the PR 🤔

@tnull tnull requested review from TheBlueMatt and dunxen September 22, 2022 14:53
@dunxen
Copy link
Contributor

dunxen commented Sep 22, 2022

I don't think that CI failure is related to the PR 🤔

Yeah seems some no-std dependency broke the MSRV. (Assuming it's the same issue I have on one PR)

@valentinewallace valentinewallace merged commit c614a27 into lightningdevkit:main Sep 22, 2022
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.

5 participants