Skip to content

Introduce a procedural macro for Nexus-specific tests#502

Merged
teisenbe merged 4 commits into
mainfrom
test-proc-macro
Dec 13, 2021
Merged

Introduce a procedural macro for Nexus-specific tests#502
teisenbe merged 4 commits into
mainfrom
test-proc-macro

Conversation

@teisenbe

Copy link
Copy Markdown
Contributor

This handles setup and teardown of ControlPlaneTestContexts, ensuring developers don't forget to call teardown.

I'm trying to think of ways we might genericize this a bit to support some of the other setup/teardown patterned objects in our tests

This handles setup and teardown of our test contexts
@david-crespo

Copy link
Copy Markdown
Contributor

Looks great. Looks like there are still some teardown calls that could be removed.

@teisenbe

Copy link
Copy Markdown
Contributor Author

Most of the remaining ones aren't for ControlPlaneTestContext. There's one case left that I know if, and it can't be migrated to the new thing because it needs the ControlPlaneTestContext to be mutable because it mucks with the producer field (dropping and re-assigning it). Though I suppose we could make the passed in ControlPlaneTestContext mutable, it originally wasn't when I was thinking I'd need to write an unwind handler and was limited by unwind-safeness

@teisenbe

Copy link
Copy Markdown
Contributor Author

Oh, I misremembered how Rust borrows work with structs. The oximeter test (the one that mucks with fields of the context) can't be migrated to use this macro since it must own the context

@davepacheco

Copy link
Copy Markdown
Collaborator

I love this -- and especially that it also makes sure the name is set usefully. I often copy/paste those and forget to fix them.

@teisenbe

teisenbe commented Dec 13, 2021

Copy link
Copy Markdown
Contributor Author

I love this -- and especially that it also makes sure the name is set usefully. I often copy/paste those and forget to fix them.

Yeah, me too

cptestctx.teardown().await;
}

#[tokio::test]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do these need to be converted to #[nexus_test]?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, thank you! Looks like I somehow only partially migrated this file

cptestctx.teardown().await;
}

#[tokio::test]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not this one because I'm using a custom config, I think

@david-crespo david-crespo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fantastic

Comment thread nexus/Cargo.toml
[dev-dependencies]
criterion = { version = "0.3", features = [ "async_tokio" ] }
expectorate = "1.0.4"
nexus-test-utils-macros = { path = "test-utils-macros" }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this one of the macros that has to be in its own crate? If so, we may as well give it a more specific crate name. If not, can we just put it into necus/test-utils?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, proc macros have to be the only thing in their crate (though you can have as many as you want in it). I believe if you try to set proc-macro = true in the Cargo.toml in a crate with other public things in it, you get an error

@davepacheco

Copy link
Copy Markdown
Collaborator

Sorry -- I thought I had submitted several comments earlier but they were still "pending" -- I guess I just separately left a single top-level comment. Anyway, you resolved most of them already. I had one nit about the crate name but feel free to land as-is if you prefer.

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.

3 participants