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

Make sure our test features depend on their dependencies test features #2627

Conversation

ndr-ds
Copy link
Contributor

@ndr-ds ndr-ds commented Oct 15, 2024

Motivation

Currently our test features are not necessarily depending on the test features from the crate's dependencies

Proposal

Fix that and make sure our test feature depends on the test feature from our dependencies

Test Plan

CI

Release Plan

  • Nothing to do / These changes follow the usual release cycle.

Copy link
Contributor Author

ndr-ds commented Oct 15, 2024

"tokio/macros",
"linera-base/test",
"linera-execution/test",
"linera-views/test",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that a problem if we don't use the testing library of linera-views ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we run cargo check-all-features in our CI, if I add a test only function to a trait for example on linera-views, when cargo check-all-features tries to check linera-chain compiled with the test feature, it will think that the test implementation of the function is missing in linera-views. That happens because linera-chain's test feature doesn't include linera-views's test feature, so the test implementation will be seen as test by linera-views but not by linera-chain, so CI will fail.
This happened here: #2607 (comment)

So I figured we might as well fix this for all dependencies already

@ndr-ds ndr-ds marked this pull request as ready for review October 15, 2024 15:44
@ndr-ds ndr-ds force-pushed the 10-15-make_sure_our_test_features_depend_on_their_dependencies_test_features branch 2 times, most recently from 21039cc to c9d5cae Compare October 15, 2024 18:44
@ndr-ds ndr-ds force-pushed the 10-15-make_sure_our_test_features_depend_on_their_dependencies_test_features branch from c9d5cae to 11d2544 Compare October 16, 2024 00:54
Copy link
Contributor Author

ndr-ds commented Oct 16, 2024

Merge activity

  • Oct 15, 11:22 PM EDT: A user started a stack merge that includes this pull request via Graphite.
  • Oct 15, 11:22 PM EDT: A user merged this pull request with Graphite.

@ndr-ds ndr-ds merged commit 8765058 into main Oct 16, 2024
7 checks passed
@ndr-ds ndr-ds deleted the 10-15-make_sure_our_test_features_depend_on_their_dependencies_test_features branch October 16, 2024 03:22
ndr-ds pushed a commit that referenced this pull request Oct 17, 2024
)

## Motivation

The docs test is failing on main

## Proposal

Revert #2627 to take a different approach on this: if we change how we're using `add_blob`, we don't need to change any dependencies!

## Test Plan

CI + I ran `test_publish.sh` locally and it succeeds

## Release Plan

- Nothing to do / These changes follow the usual release cycle.
@ndr-ds ndr-ds linked an issue Oct 17, 2024 that may be closed by this pull request
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.

Replace ApplicationDescription by blobs
3 participants