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

Tgrade custom multitest #180

Merged
merged 14 commits into from
Sep 29, 2021
Merged

Tgrade custom multitest #180

merged 14 commits into from
Sep 29, 2021

Conversation

ethanfrey
Copy link
Contributor

@ethanfrey ethanfrey commented Sep 28, 2021

Closes #144

Requires CosmWasm/cw-plus#456 (and pointing Cargo.toml to a release)

Follow up (in #182):

  • Add multitest for tgrade_valset with Promotion, EndBlock, and MintTokens
  • Better unit test coverage

@ethanfrey ethanfrey marked this pull request as ready for review September 29, 2021 09:19
@@ -1,13 +1,17 @@
mod gov;
mod hooks;
mod msg;
#[cfg(feature = "multitest")]
Copy link

Choose a reason for hiding this comment

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

I wonder if this
#[cfg(all(test, feature = "multitest"))]
wouldn't work better - those new multitest dependencies could be moved to [dev-dependencies] if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... we could improve this flag (I would prefer to do in another PR so there is a much smaller diff).

If package A imports package B in test mode, are the dev-dependencies of package B included?

I understood it would use the dev-dependencies of the package being tested (A) and the normal dependencies of the other packages. Also #[cfg(test)] flag on the dependencies are exposed to the caller?

Again, I would prefer to merge and get #182 in (which uses these features from another package), then happy to review/approve a PR that does a less intrusive flagging of these deps

Copy link
Contributor

Choose a reason for hiding this comment

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

I am pretty sure no - cfg(test) is set if and only if your crate is under test. You want those modules to be exported for testing in other modules (those are helpers for other modules, not this particular one).

Copy link
Contributor

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

I really appreciate this change. Just wanted to take #88, but I was missing this. I see some todos but I guess to be implemented as follow up.

@@ -1,13 +1,17 @@
mod gov;
mod hooks;
mod msg;
#[cfg(feature = "multitest")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I am pretty sure no - cfg(test) is set if and only if your crate is under test. You want those modules to be exported for testing in other modules (those are helpers for other modules, not this particular one).

@ethanfrey ethanfrey merged commit d6cfed1 into main Sep 29, 2021
@ethanfrey ethanfrey deleted the tgrade-custom-multitest branch September 29, 2021 10:41
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.

Add CustomHandler for TgradeMsg/TgradeQuery
3 participants