Files that changed from the base of the PR and between 5785d64 and ff58c44.

• .github/workflows/tests.yaml (5 hunks)
• scripts/run-local-anvil.sh (1 hunks)
• tests/conftest.py (7 hunks)

.github/workflows/tests.yaml (1)

103-111: Improved snapshot handling and verification.

The changes to the snapshot download and verification process are well-implemented:

1. The use of the --recursive flag in the gcloud storage rsync command ensures all files and subdirectories are synced.
2. The standardized destination path (smoke_test_snapshot) improves consistency.
3. The addition of the "Check snapshot" step helps verify the download's success.

These improvements enhance the reliability and consistency of the snapshot handling process.

tests/conftest.py (3)

7-7: LGTM: Import statements updated

The addition of the re module import and the inclusion of CalledProcessError in the subprocess import are appropriate for the new run_hopli_cmd function introduced later in the file.

Also applies to: 11-11

---

127-127: LGTM: Node 6 configuration updated for lower winning probability

The change of Node 6's configuration file to "barebone-lower-win-prob.cfg.yaml" aligns with the PR objective of testing lowered winning probabilities. This modification ensures that Node 6 is set up correctly for the new test scenarios.

---

218-220: LGTM: Improved logging in snapshot_reuse function

The addition of logging for copying the anvil state file enhances the visibility of the snapshot reuse process. This will help with debugging and understanding the test setup process.

Files that changed from the base of the PR and between ff58c44 and 54e6bf6.

• .github/workflows/tests.yaml (5 hunks)
• tests/conftest.py (6 hunks)

.github/workflows/tests.yaml (5)

84-88: Consider pinning the Cloud SDK to an exact version.

While adding the Cloud SDK setup step is a good improvement, it's recommended to pin to an exact version instead of using a minimum version constraint. This ensures consistent behavior across different environments and CI runs.

As suggested in a previous review:

- name: Set up Cloud SDK
  uses: google-github-actions/setup-gcloud@v2
  with:
    version: '363.0.0'

This change would improve reproducibility and prevent potential issues from unexpected updates.

---

103-108: Improved snapshot download process.

The changes to the snapshot download step are beneficial:

1. Standardizing the snapshot storage path to smoke_test_snapshot/snapshot improves consistency and makes it easier to manage snapshots across different test suites.
2. Adding the --recursive flag to the gcloud storage rsync command ensures that all nested directories and files are properly synchronized, which is crucial for comprehensive snapshot management.

These improvements align with best practices for cloud storage operations and should enhance the reliability of the snapshot download process.

---

117-123: Improved snapshot upload configuration.

The changes to the snapshot upload step are well-considered:

1. Changing the destination path to hoprnet-test-artifacts/smoke_test_snapshot aligns with the standardization implemented in the download step. This consistency improves the overall organization and manageability of test artifacts.

2. Setting gzip: false is appropriate for snapshot files. This ensures that the files are uploaded without additional compression, which is beneficial for snapshots that may already be compressed or contain binary data.

These modifications enhance the reliability and efficiency of the snapshot upload process.

---

159-162: Expanded test coverage with new test suites.

The addition of new test suites ('redeeming', 'rest_api', and 'win_prob') to the matrix strategy is a significant improvement:

1. It expands the coverage of the smoke tests, enhancing the overall quality assurance process.
2. The 'win_prob' suite aligns perfectly with the PR objective of adding E2E tests for lowered winning probabilities.
3. The 'rest_api' suite suggests improved testing of API endpoints, which is crucial for ensuring the reliability of the application's interface.
4. The 'redeeming' suite likely focuses on ticket redemption processes, which is important given the changes in winning probabilities.

These additions demonstrate a comprehensive approach to testing, covering various aspects of the system's functionality.

---

177-180: Consistent improvements across jobs, with a suggestion for Cloud SDK setup.

The changes to the tests-smoke job mirror those in the tests-smoke-websocket job, which is excellent for maintaining consistency across the workflow:

1. The addition of the Cloud SDK setup step ensures that necessary tools are available for both jobs.
2. The modifications to snapshot download and upload steps standardize the process across different test suites.

These changes improve the overall structure and reliability of the workflow. However, as mentioned earlier, consider pinning the Cloud SDK to an exact version for better reproducibility:

- name: Set up Cloud SDK
  uses: google-github-actions/setup-gcloud@v2
  with:
    version: '363.0.0'

This change would ensure consistent behavior across different environments and CI runs for both jobs.

Also applies to: 196-201, 210-216

tests/conftest.py (2)

127-127: LGTM: Removed NETWORK2 references and updated Node 6 configuration

The changes align well with the PR objectives:

1. Node 6's configuration file has been updated to "barebone-lower-win-prob.cfg.yaml", which supports testing lowered winning probabilities.
2. The removal of commented-out code related to NETWORK2 improves code cleanliness.

These modifications effectively streamline the testing framework for the new scenarios with lowered winning probabilities.

Also applies to: 511-512

---

279-291: LGTM: Updated snapshot validation process

The changes to the snapshot_usable function improve the snapshot validation process by:

1. Including checks for new configuration files (e.g., "barebone-lower-win-prob.cfg.yaml").
2. Ensuring that all necessary node-specific files are present in the snapshot.

These updates enhance the reliability of the snapshot system, which is crucial for efficient and consistent testing.

Files that changed from the base of the PR and between 54e6bf6 and 5407b64.

• tests/conftest.py (7 hunks)

tests/conftest.py (2)

7-7: LGTM: New imports added

The addition of re and CalledProcessError imports suggests new functionality related to regular expressions and subprocess error handling. These changes align with the new run_hopli_cmd function introduced later in the file.

Also applies to: 11-11

---

127-127: LGTM: Updated Node 6 configuration for lower winning probability

The change of Node 6's configuration file to "barebone-lower-win-prob.cfg.yaml" aligns with the PR objective of testing lowered winning probabilities. This modification ensures that Node 6 is set up correctly for the new test scenarios.

Files that changed from the base of the PR and between 5407b64 and 3a481af.

• tests/conftest.py (9 hunks)

tests/conftest.py (5)

7-7: Importing re module is appropriate

The addition of import re is necessary for the regular expression operations used later in the code.

---

127-127: Updated configuration for Node 6 aligns with PR objectives

Changing Node 6's configuration to "barebone-lower-win-prob.cfg.yaml" is consistent with the goal of testing lowered winning probabilities.

---

147-149: Function renaming accurately reflects its purpose

Renaming the function to nodes_with_lower_outgoing_win_prob makes its purpose clearer, indicating that it returns nodes with a lower outgoing ticket winning probability.

---

283-298: Snapshot verification enhancements are valid

Including protocol-config.json and node-specific files in expected_files improves the robustness of snapshot verification. The iterative check ensures all necessary files are present.

---

352-354: Addition of is_node_ready function improves readiness checks

Introducing the is_node_ready function provides a more granular approach to determine when a node is fully ready, enhancing the reliability of the startup process.

Also applies to: 359-359

Files that changed from the base of the PR and between 5ad20ee and 6e3e69b.

• tests/test_redeeming.py (1 hunks)

tests/test_redeeming.py (6)

15-21: Well-structured test for unredeemed tickets without message transmission

The test test_hoprd_should_not_have_unredeemed_tickets_without_sending_messages correctly verifies that a node has no unredeemed tickets before any messages are sent. The use of parameterization ensures coverage across different nodes.

---

136-136: ⚠️ Potential issue

Ensure integer division when calculating message_count

Similar to line 58, ensure that integer division is used to prevent accidental float-to-integer conversion which can lead to incorrect message_count.

Apply this diff:

-message_count = int(TICKET_AGGREGATION_THRESHOLD / 10)
+message_count = TICKET_AGGREGATION_THRESHOLD // 10

Likely invalid or redundant comment.

---

160-160: 🛠️ Refactor suggestion

Avoid using fixed sleep durations in tests

Using await asyncio.sleep(10) can lead to longer test times and flakiness. It's better to wait for a specific condition, such as checking if aggregation has completed.

Replace the fixed sleep with a polling mechanism:

-await asyncio.sleep(10)  # wait for aggregation to finish before redeeming
+timeout = 30
+start_time = asyncio.get_event_loop().time()
+while True:
+    # Implement a condition check appropriate for your aggregation completion
+    if await is_aggregation_complete(swarm7[route[1]]):
+        break
+    elif asyncio.get_event_loop().time() - start_time > timeout:
+        raise TimeoutError("Aggregation did not complete in time")
+    await asyncio.sleep(1)

Ensure that you implement is_aggregation_complete to accurately check the aggregation status.

Likely invalid or redundant comment.

---

182-184: ⚠️ Potential issue

Add validation to prevent IndexError due to insufficient route length

Accessing route[1] and route[-1] assumes that route has at least two elements. If route is shorter, this will raise an IndexError.

Apply this diff to add a safeguard:

+assert len(route) >= 2, "Route must contain at least two nodes"
 await send_and_receive_packets_with_pop(
     packets, src=swarm7[route[0]], dest=swarm7[route[-1]], path=[swarm7[route[1]].peer_id]
 )

Likely invalid or redundant comment.

---

93-96: 🧹 Nitpick (assertive)

Correct the indentation of the multi-line assertion

The assertion spanning lines 93-96 is over-indented, which may affect readability and could potentially lead to misunderstandings of the code structure.

Apply this diff to adjust the indentation:

 assert (
-                   balance_str_to_int(statistics_after_redemption.redeemed_value)
-                   - balance_str_to_int(statistics_after.redeemed_value)
-           ) == (len(packets) * TICKET_PRICE_PER_HOP)
+    balance_str_to_int(statistics_after_redemption.redeemed_value)
+    - balance_str_to_int(statistics_after.redeemed_value)
+) == (len(packets) * TICKET_PRICE_PER_HOP)

Likely invalid or redundant comment.

---

23-51: 🛠️ Refactor suggestion

Consider limiting retries in channel_redeem_tickets to prevent potential infinite loops

The channel_redeem_tickets function contains a while True loop without a termination condition other than success. While this ensures eventual completion if the channel redeems successfully, it could lead to an infinite loop if the redemption continuously fails.

Apply this diff to limit the number of retries:

 async def channel_redeem_tickets(api: HoprdAPI, channel: str):
+    max_retries = 40
+    retries = 0
     while True:
         if await api.channel_redeem_tickets(channel):
             break
         else:
             await asyncio.sleep(0.5)
+            retries += 1
+            if retries >= max_retries:
+                raise Exception("Max retries reached while redeeming tickets")

Likely invalid or redundant comment.