Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #789 +/- ##
==========================================
+ Coverage 76.33% 76.85% +0.52%
==========================================
Files 73 73
Lines 20293 20520 +227
==========================================
+ Hits 15490 15770 +280
+ Misses 4803 4750 -53 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @sehkone — this PR (Add testcode to network group behavior, @pott-cho) adds tests clarifying customer-network add/update behavior and closes #775. All CI checks and codecov are green. The branch is mergeable but blocked because GitHub requires at least one approving review from someone with write access and no reviewers are currently assigned. Could you please assign a reviewer or let us know if there's a reason to hold (release timing, broader review plan, etc.)? If you want, I can suggest reviewers or provide any extra context to speed review. |
|
Hi @sehkone — this PR (#789, author: @pott-cho) adds tests clarifying customer-network add/update behavior and closes #775. All CI checks and codecov are green and the branch is mergeable, but GitHub requires at least one approving review from someone with write access and no reviewers are currently assigned. Could you please assign a reviewer with write access or let us know if there’s a reason to hold this (release timing, broader review plan, etc.)? If helpful, I can suggest reviewers familiar with the networking area to speed review. Thanks! |
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn allow_duplicate_network_groups_on_insert() { |
There was a problem hiding this comment.
I think adding assertions that verify the network entries are actually preserved as intended (e.g., both n1 and n2 exist) would improve the test’s purpose. Similar assertions might be helpful to allow_duplicate_network_groups_on_update as well.
|
|
||
| #[tokio::test] | ||
| async fn ensure_network_group_items_are_sorted() { | ||
| // The values inside networkGroup are stored in alphabetical order regardless of the input order. |
There was a problem hiding this comment.
| // The values inside networkGroup are stored in alphabetical order regardless of the input order. | |
| // The values inside networkGroup are stored in deterministic sorted order (per type-defined ordering), regardless of input order. |
“alphabetical order” is not a perfectly precise term here. To avoid inaccuracies and keep the comment future-proof against implementation changes, please consider the suggestion above.
a156659 to
41c9495
Compare
sophie-cluml
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier test concern.
Two small follow-ups:
- The recursion-limit change in
examples/minireview.rsseems orthogonal to #775. Would it make sense to stack this PR on top of #802 instead? - The new insert/update tests now assert that two network entries remain, but they still do not verify that the preserved entries are actually
n1andn2. Tightening those assertions would make the test intent more direct.
16d628a to
f6b61f3
Compare
I removed the recursion-limit change because it is already handled in #802. I also tightened the tests by adding assertions to verify that the preserved entries are indeed |
Closes: #775
I added test code to clarify the behavior of customer network add/update.
The tests in this PR clarify the following behaviors:
networkGroupfields (hosts,networks,ranges) are stored in sorted order.networkGroupare removed without an error.networkGroupvalues across different network entries are allowed (for both insert and update).