Skip to content

faucet: put unique port range for tests behind debug_assertions gate#7847

Closed
puhtaytow wants to merge 1 commit intoanza-xyz:masterfrom
puhtaytow:fix-unused-import-in-release-build-faucet
Closed

faucet: put unique port range for tests behind debug_assertions gate#7847
puhtaytow wants to merge 1 commit intoanza-xyz:masterfrom
puhtaytow:fix-unused-import-in-release-build-faucet

Conversation

@puhtaytow
Copy link
Copy Markdown

@puhtaytow puhtaytow commented Sep 3, 2025

Problem

The #7736 had missing gate for import used only in debug_assertions case, causing warning in release builds.

Summary of Changes

As suggested here #7736 (comment)
Now its properly gated.

Comment thread faucet/src/faucet.rs Outdated
@puhtaytow puhtaytow force-pushed the fix-unused-import-in-release-build-faucet branch from f639519 to 90ce42f Compare September 3, 2025 07:02
@puhtaytow puhtaytow force-pushed the fix-unused-import-in-release-build-faucet branch from 90ce42f to c6928d6 Compare September 3, 2025 07:03
Copy link
Copy Markdown

@alexpyattaev alexpyattaev left a comment

Choose a reason for hiding this comment

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

LGTM

@alexpyattaev
Copy link
Copy Markdown

@gregcusack are you happy with this fix?

Copy link
Copy Markdown

@gregcusack gregcusack left a comment

Choose a reason for hiding this comment

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

lgtm! but would wait for steve review to make sure he is in fact ok with this in line gating

@puhtaytow
Copy link
Copy Markdown
Author

thank you guys for the review 🙏

@gregcusack gregcusack added the CI Pull Request is ready to enter CI label Sep 3, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Sep 3, 2025
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.1%. Comparing base (2933846) to head (c6928d6).
⚠️ Report is 17 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7847   +/-   ##
=======================================
  Coverage    83.0%    83.1%           
=======================================
  Files         809      809           
  Lines      356483   356483           
=======================================
+ Hits       296203   296244   +41     
+ Misses      60280    60239   -41     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Let's all agree on a way forward in: #7736 (comment)

My latest rec. was to back out that PR, so if we go that way we should avoid adding a commit that is on top of the commit that we'd be backing out

@steviez
Copy link
Copy Markdown

steviez commented Sep 4, 2025

We backed out #7736 with #7879 so this fixup on #7736 is no longer needed/valid; closing the PR

@steviez steviez closed this Sep 4, 2025
@puhtaytow puhtaytow deleted the fix-unused-import-in-release-build-faucet branch September 6, 2025 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants