unit tests: Add mz_ore::test to initialize logging implicitly #19603

def- · 2023-05-31T13:45:49Z

It seems easy to forget to initialize logging. Unfortunately there is no easy way of doing this in Rust currently. See for example a recent Slack discussion: https://materializeinc.slack.com/archives/C0246GEHL8N/p1683754885066559

I'll rebase this once the reviews are through since it has huge conflict potential.

Motivation

This PR refactors existing code.

Tips for reviewer

Review individual commits.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:

benesch

This is a fantastic idea! Let's have @guswynn or @petrosagg or some other Rust expert review the proc macro itself, but +100 on the idea from me.

Can we do a blanket find/replace on #[test] in the codebase, too? It seems that you've limited the swap for just those tests that mz_ore::test::init_logging() was called in, but really any test function that doesn't call that is a recipe for trouble. Then we should also update STYLE.md to say that use of this macro is mandatory.

def- · 2023-06-01T17:47:16Z

.github/CODEOWNERS

@@ -84,6 +84,7 @@
 /src/stash-debug                    @MaterializeInc/surfaces
 /src/storage                        @MaterializeInc/storage
 /src/storage-client                 @MaterializeInc/storage
+/src/test-macro                     @MaterializeInc/qa


@philip-stoev I hope this is ok, otherwise can also assign it to myself directly.

Yes, it is ok, thanks.

def- · 2023-06-01T17:48:40Z

src/test-macro/src/lib.rs

+/// Copyright (C) 2019-2022 Daniel Mueller <[email protected]>
+/// SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+#[proc_macro_attribute]


This is the actually interesting part of this PR, the rest is just to actually use it.

def- · 2023-06-01T17:49:09Z

src/test-macro/src/lib.rs

+          #body
+        }
+
+        #logging_init


If we later have other things we want to run in each unit test, during setup or at the end, we can add them here in the future.

guswynn · 2023-06-01T19:08:50Z

src/test-macro/src/lib.rs

+    let inner_test = match args.as_slice() {
+        [] => parse_quote! { ::core::prelude::v1::test },
+        [NestedMeta::Meta(Meta::Path(path))] => quote! { #path },
+        [NestedMeta::Meta(Meta::List(list))] => quote! { #list },
+        _ => panic!("unsupported attributes supplied: {:?}", args),
+    };


This doesn't support flavor = "multi_thread" (https://docs.rs/tokio/latest/tokio/attr.test.html#multi-threaded-runtime) in tokio::test does it? What if we expanded to tokio::test over the normal test attribute automatically if the function is async? Then we can also support flavor as an arg

It does, I used the flavor = "multi_threads" in some tests and haven't seen a problem. Example from the project I took this from: https://github.com/d-e-s-o/test-log/blob/f500323f947a4bfdcb015edbc814555ddb04a0fc/tests/mod.rs#L71

Oh then this looks good!

philip-stoev

Thank you. Can we get one of the linters to start rejecting incorrect direct use of the test macro? I believe @teskje was the last person that was trying to use a linter to prohibit certain constructs altogether.

def- · 2023-06-02T08:10:46Z

I don't know how to create Rust-parser based warnings myself. I can easily do a regex though if that is sufficient.

Edit: I found clippy.toml which looks good for this.

teskje · 2023-06-02T08:25:53Z

Clippy's disallowed-macros config seems like it should work for this, but I tried for a bit and couldn't get it to flag #[test] or #[tokio::test].

teskje

LGTM, though I'll also leave the proc macro review to more knowledgable folks.

I agree that it would be nice to have something in CI flagging use of non-mz test macros.

def- · 2023-06-02T08:42:45Z

clippy is unfortunately insufficient here, since it will complain about the location where the mz_ore::test macro is used, so we'd have to silence the warning everywhere, which defeats the purpose. I tried with:

diff --git a/clippy.toml b/clippy.toml
index 83b1777a60..c92db4b1ee 100644
--- a/clippy.toml
+++ b/clippy.toml
@@ -49,6 +49,9 @@ disallowed-methods = [
 disallowed-macros = [
     { path = "proptest::prop_oneof", reason = "use `proptest::strategy::Union::new` instead" },
     { path = "log::log", reason = "use the macros provided by `tracing` instead (#9992)" },
+    { path = "core::prelude::v1::test", reason = "use the `mz_ore::test` macro instead" },
+    # How to warn about tokio::test but not mz_ore::test(tokio::test)?
+    #{ path = "tokio::test", reason = "use the `mz_ore::test(tokio::test)` macro instead" },
 ]

 disallowed-types = [
diff --git a/src/test-macro/src/lib.rs b/src/test-macro/src/lib.rs
index 4748331929..3cd0ec08b7 100644
--- a/src/test-macro/src/lib.rs
+++ b/src/test-macro/src/lib.rs
@@ -104,6 +104,7 @@ pub fn test(attr: TokenStream, item: TokenStream) -> TokenStream {
     let input = parse_macro_input!(item as ItemFn);

     let inner_test = match args.as_slice() {
+        #[allow(clippy::disallowed_macros)]
         [] => parse_quote! { ::core::prelude::v1::test },
         [NestedMeta::Meta(Meta::Path(path))] => quote! { #path },
         [NestedMeta::Meta(Meta::List(list))] => quote! { #list },

philip-stoev · 2023-06-02T08:52:20Z

Ok then so the new boilerplate code will desseminate using copy+paste then.

def- · 2023-06-02T09:01:48Z

misc/lint/test-attribute.sh

@@ -0,0 +1,50 @@
+#!/usr/bin/env bash


As requested, added a lint. Sample output:

lint: error: test-attribute: src/ore/tests/future.rs: use of disallowed `#[tokio::test] attribute. Use the `#[mz_ore::test(tokio::test)]` attribute instead or add a `// allow(test-attribute)` comment

So that logging will be enabled without any extra action required Except in ore directory, since the macro refers to mz-ore crate, which is not available.

Requires detection of whether we are inside mz-ore currently in the macro

def- · 2023-06-02T18:03:01Z

I'll set this to auto-merge unless someone complains and/or wants to review the macro. After it's merged I'll check the open PRs to see if they would break linting and notify people.

def- force-pushed the pr-mz-test branch 3 times, most recently from 1464c3d to d102f73 Compare May 31, 2023 14:28

benesch reviewed Jun 1, 2023

View reviewed changes

def- force-pushed the pr-mz-test branch from d102f73 to 61d69a8 Compare June 1, 2023 17:42

def- changed the title ~~unit tests: Add mz_test::test to initialize logging implicitly~~ unit tests: Add mz_ore::test to initialize logging implicitly Jun 1, 2023

def- requested review from petrosagg, philip-stoev, guswynn and nrainer-materialize June 1, 2023 17:46

def- commented Jun 1, 2023

View reviewed changes

def- force-pushed the pr-mz-test branch from 4c95fd4 to c2452dc Compare June 1, 2023 18:09

def- marked this pull request as ready for review June 1, 2023 18:09

def- requested review from a team June 1, 2023 18:09

def- requested review from a team, aalexandrov and umanwizard as code owners June 1, 2023 18:09

def- requested a review from benesch June 1, 2023 18:09

guswynn reviewed Jun 1, 2023

View reviewed changes

def- force-pushed the pr-mz-test branch from 4e30fa0 to 59af408 Compare June 1, 2023 21:22

philip-stoev approved these changes Jun 2, 2023

View reviewed changes

nrainer-materialize approved these changes Jun 2, 2023

View reviewed changes

teskje approved these changes Jun 2, 2023

View reviewed changes

def- commented Jun 2, 2023

View reviewed changes

def- added 9 commits June 2, 2023 18:01

unit tests: Add mz_test::test to initialize logging implicitly

32a49e4

unit tests: Use mz-ore::test macro in all tests

3861a19

So that logging will be enabled without any extra action required Except in ore directory, since the macro refers to mz-ore crate, which is not available.

unit tests: Also enable mz-ore::test macro in ore directory

433d2b5

Requires detection of whether we are inside mz-ore currently in the macro

Make linter happy

f70040d

Update documentation

06e0f88

test_emit_tracing_notice requires tracing not being initialized

667dd7d

Update style.md with actual examples of mz_ore::test

76aba60

lint: Add test-attribute.sh check

5668737

Fix linter after rebase

640ca34

def- force-pushed the pr-mz-test branch from d8bc934 to 640ca34 Compare June 2, 2023 18:02

def- enabled auto-merge June 2, 2023 18:03

This was referenced Jun 2, 2023

storage: make rocksdb batch size configurable #19509

Merged

[DNM] WIP cache sim #19532

Closed

def- merged commit 9dcb55c into MaterializeInc:main Jun 2, 2023

def- deleted the pr-mz-test branch June 2, 2023 19:21

def- mentioned this pull request Jun 2, 2023

cargo-test: Use mz_ore::test in new adt timestamp tests #19685

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unit tests: Add mz_ore::test to initialize logging implicitly #19603

unit tests: Add mz_ore::test to initialize logging implicitly #19603

def- commented May 31, 2023 •

edited

Loading

benesch left a comment

def- Jun 1, 2023

philip-stoev Jun 2, 2023

def- Jun 1, 2023

def- Jun 1, 2023

guswynn Jun 1, 2023

def- Jun 1, 2023

guswynn Jun 1, 2023

philip-stoev left a comment

def- commented Jun 2, 2023 •

edited

Loading

teskje commented Jun 2, 2023

teskje left a comment •

edited

Loading

def- commented Jun 2, 2023 •

edited

Loading

philip-stoev commented Jun 2, 2023

def- Jun 2, 2023 •

edited

Loading

nrainer-materialize Jun 2, 2023

def- commented Jun 2, 2023 •

edited

Loading

unit tests: Add mz_ore::test to initialize logging implicitly #19603

unit tests: Add mz_ore::test to initialize logging implicitly #19603

Conversation

def- commented May 31, 2023 • edited Loading

Motivation

Tips for reviewer

Checklist

benesch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philip-stoev left a comment

Choose a reason for hiding this comment

def- commented Jun 2, 2023 • edited Loading

teskje commented Jun 2, 2023

teskje left a comment • edited Loading

Choose a reason for hiding this comment

def- commented Jun 2, 2023 • edited Loading

philip-stoev commented Jun 2, 2023

def- Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

def- commented Jun 2, 2023 • edited Loading

def- commented May 31, 2023 •

edited

Loading

def- commented Jun 2, 2023 •

edited

Loading

teskje left a comment •

edited

Loading

def- commented Jun 2, 2023 •

edited

Loading

def- Jun 2, 2023 •

edited

Loading

def- commented Jun 2, 2023 •

edited

Loading