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

fix(test): test clients should log to data_dir #813

Merged
merged 5 commits into from Oct 11, 2023

Conversation

RolandSherwin
Copy link
Member

@RolandSherwin RolandSherwin commented Oct 9, 2023

Description

Summary generated by Reviewpad on 09 Oct 23 09:43 UTC

This pull request includes changes to multiple files. Here is a summary of the diff:

  1. storage_payments.rs:
  • Added a new function init_logging() that initializes logging to the data directory.
  • The function checks if the SN_LOG environment variable is provided and overrides the logging targets accordingly.
  • If the data directory exists, the function creates a log file with a timestamp in the directory data_dir/safe/client/logs.
  • If the data directory does not exist, logging is set to stdout.
  • The function init_logging() is called multiple times in different test functions.
  • Overall, the changes seem to be related to storage payment functionalities and logging.
  1. msgs_over_gossipsub.rs:
  • Updated the import statement common::safenode_proto to crate::common::safenode_proto.
  • Moved the import statement for sn_node::NodeEvent to a different location.
  • Removed the usage of the NodeEvent type from the sn_node module.
  • Added the import statement for eyre::Result.
  • Some changes related to networking, including the definition of IP address and socket address.
  1. Another file (not specified in the diff summary):
  • Added an import statement for the init_logging function from the common module.
  • Removed an import statement for the init_logging function from the sn_logging module.
  • Removed an import statement for the Level type from the tracing_core module.
  • Added a call to the init_logging function.
  • Removed several lines related to setting up logging targets and log appender guard.
  • Overall, the changes seem to be focused on modifying the logging configuration in the data_availability_during_churn test function.
  1. sequential_transfers.rs:
  • Removed unused imports (mod common, use sn_transfers::NanoTokens).
  • Moved the import statement for common::{get_client_and_wallet, get_wallet, init_logging} to a different location.
  • Reordered the import statements for sn_client::send and sn_transfers::{create_offline_transfer, rng, Hash, UniquePubkey}.
  • Marked the test function cash_note_transfer_multiple_sequential_succeed as an async function.
  • The changes seem to be focused on cleaning up unused imports and reorganizing the import statements.
  1. verify_data_location.rs:
  • Reorganized and updated imports.
  • Modified the logging functionality, adjusting log targets and log levels.
  • Modified the function verify_data_location.
  • Removed the variable tmp_dir.
  • Replaced the variable logging_targets with a call to init_logging.
  • Updated the variable churn_count.
  • Overall, the changes seem to be significant and might have an impact on the functionality of the code. Please review them carefully.
  1. nodes_rewards.rs:
  • Modified import statements, including adding new import statements and rearranging existing ones.
  • Added the function init_logging() before the test function nodes_rewards_for_storing_chunks().
  • Added the test function nodes_rewards_for_storing_registers() that includes a call to init_logging().
  • Please review these changes in the test file nodes_rewards.rs.

Please review these file diffs and ensure they meet the requirements of the code.

@reviewpad reviewpad bot added the Medium Medium sized PR label Oct 9, 2023
@RolandSherwin RolandSherwin force-pushed the improve_test_client_logs branch 2 times, most recently from 21a1b0b to bebee1a Compare October 9, 2023 11:23
@RolandSherwin RolandSherwin marked this pull request as ready for review October 9, 2023 11:24
@reviewpad reviewpad bot requested a review from bochaco October 9, 2023 11:24
@reviewpad
Copy link

reviewpad bot commented Oct 9, 2023

Reviewpad Report

‼️ Errors

  • Unconventional commit detected: 'fix(test): return log WorkerGuard
  • the log file is empty if the guard is dropped' (3d9614b)
  • Unconventional commit detected: 'fix(log): capture logs from tests
  • if test-threads=1, only the logs from the test that calls init_logging() first are written
  • this is because of the use of Once::new() during init_logging for tests' (191880d)

@reviewpad reviewpad bot added Large Large sized PR and removed Medium Medium sized PR labels Oct 9, 2023
let log_guard = tracing_subscriber::registry()
.with(layers.layers)
.set_default();
// this is the test_name and not the test_file_name; TODO: should try passing as logging's target filter

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
@RolandSherwin RolandSherwin force-pushed the improve_test_client_logs branch 2 times, most recently from c5dd013 to 5b2124d Compare October 9, 2023 14:06
Copy link
Member

@grumbach grumbach left a comment

Choose a reason for hiding this comment

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

LGTM

@grumbach grumbach added this pull request to the merge queue Oct 10, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 10, 2023
@RolandSherwin RolandSherwin added this pull request to the merge queue Oct 10, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 10, 2023
- the log file is empty if the guard is dropped
- if test-threads=1, only the logs from the test that calls init_logging() first are written
- this is because of the use of Once::new() during init_logging for tests
@RolandSherwin RolandSherwin added this pull request to the merge queue Oct 11, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 11, 2023
@joshuef joshuef merged commit 974a18f into maidsafe:main Oct 11, 2023
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Large Large sized PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants