Skip to content

tracing追加#571

Merged
cipepser merged 14 commits intomasterfrom
feature/tracing-errors
Apr 24, 2021
Merged

tracing追加#571
cipepser merged 14 commits intomasterfrom
feature/tracing-errors

Conversation

@cipepser
Copy link
Copy Markdown
Collaborator

  • エラーログをテストできるようtracingを追加
  • tests-utilにstd/sgxのfeatures追加

次PR

  • テスト全体にtracing仕込む(本PRではサンプルとして1箇所だけlogs_containを追加)

@cipepser cipepser marked this pull request as ready for review April 23, 2021 11:44
let balance: state_runtime_node_api::state::get::Response = test::read_body_json(resp).await;
assert_eq!(balance.state, 90);

assert!(logs_contain("ERROR"));
Copy link
Copy Markdown
Member

@osuketh osuketh Apr 23, 2021

Choose a reason for hiding this comment

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

ここは、ログに ERROR が含まれている場合、パニックしてほしい意図です??

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

です!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

asser!はfalseでpanicしますよ!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

あれ、たしかに...取れていない...?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

すいません..意図が逆でした :bow_tateburu:
ここでは残高を上回るようなinvalidなtxを実行しているので「 ERROR が含まれている」ことをチェックしたい意図です。
なので 「 ERROR が含まれて いない 」場合にpanicさせたく、もともとのやつが合ってそうです

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

fmfm、なるほどです。 その場合、特定の状態遷移で ERROR が含まれていることを検証したいが、途中でpanicさせてしまうと後続のtest処理が実行されなくなってしまうので、ERROR 数をカウントして、最後にカウント数バリデーションするみたいな感じがよさそうですかね。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

最終的にはそのほうがよさそうですね
そのためには、このissue潰さないといけないですが...

#570

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

そうですね! そうすると、task順的には、本PR(コノママデ)-> #570 -> ERRORカウントテストに変更 -> 他testにtracing_test導入 になりますかね〜

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

それでいきましょう!!

use tracing_subscriber::fmt::MakeWriter;

lazy_static! {
pub static ref GLOBAL_TRACING_BUF: Mutex<Vec<u8>> = Mutex::new(vec![]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

GLOBAL_TRACING_BUF の中身を空にする関数も用意しておいて、 ERRORログを意図する状態遷移がくるタイミングで同期までsleepして、 logs_contain() で検証 -> GLOBAL_TRACING_BUFをemptyに -> テストの続きを実行。待ってもERROR来なかったらパニック。のような処理を追加できると良さげですかね〜

Copy link
Copy Markdown
Member

@osuketh osuketh Apr 23, 2021

Choose a reason for hiding this comment

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

ERRORしか検証しないのであれば、error_log_contain() で良いのかも。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

空にするやつどこかで実装しようと思ってたので、シュッとこのPRで追加しますね
(tracingのscopeやtargetをいい感じにするのとどっちがいいか悩んでたんですが、テスト用のutilなので空にしてしまうでよさそう)

ERRORしか検証しないのであれば
今は ERROR で拾っているんですが、次PRで具体的なエラー文言で引っ掛ける予定なので、このままでおなしゃす

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

追加したので、ご確認おなしゃす。
log_contains の位置も110送った直後に変更しました。

assert!(resp.status().is_success(), "response: {:?}", resp);
let balance: state_runtime_node_api::state::get::Response = test::read_body_json(resp).await;
assert_eq!(balance.state, 100);
assert!(logs_contain("ERROR"));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

一見ややこしいので、 helper関数にassert!まで委譲してしまって、 panic_if_logs_contain(), panic_if_logs_not_contain() にしても良さそうですね(後々

@cipepser cipepser merged commit bfc4b73 into master Apr 24, 2021
@cipepser cipepser deleted the feature/tracing-errors branch April 24, 2021 01:05
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.

2 participants