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

bindeps: clean up metadata tests #10407

Open
ehuss opened this issue Feb 22, 2022 · 3 comments
Open

bindeps: clean up metadata tests #10407

ehuss opened this issue Feb 22, 2022 · 3 comments
Labels
Z-bindeps Nightly: binary artifact dependencies

Comments

@ehuss
Copy link
Contributor

ehuss commented Feb 22, 2022

The tests workspace_metadata_with_dependencies_no_deps_artifact and workspace_metadata_with_dependencies_no_deps are very similar. Do they need to be separate tests? Should workspace_metadata_with_dependencies_no_deps maybe be reverted back to the original workspace_metadata_no_deps test (that is, without the artifact dependencies)?

cc @Byron

@ehuss ehuss added the Z-bindeps Nightly: binary artifact dependencies label Feb 22, 2022
@Byron
Copy link
Member

Byron commented Feb 23, 2022

In #10061 another one of these tests was added to show the output of metadata with multideps enabled called workspace_metadata_with_dependencies_and_resolve_multidep.

I believe the issue could be fixed in various ways, and I am happy with the suggestions made and remove both of the new tests.

However, as the tests were created to understand how cargo metadata displays artifact deps also in conjunction with multideps, I'd lean towards keeping the most complex test (bindeps + multidep) instead as it shows that it's not breaking and produces decent output while serving as a basis for improvement at a later time.

With your direction I believe a this issue can be fixed swiftly as part of RFC-3176. Thank you.

@joshtriplett
Copy link
Member

@Byron Given the complexity of adding this, I don't think you need to remove any tests, at least until multidep gets merged. However, it may make sense to re-add a rolled back version of workspace_metadata_no_deps to make sure non-artifact-dependency cases keep working.

@Byron
Copy link
Member

Byron commented Mar 15, 2022

Thanks for letting me know. I will remove the artifact from workspace_metadata_with_dependencies_no_deps() and leave the other two tests as is. Actually I wasn't aware anymore that I changed the original test, I think that's more of an accident that is now going to be fixed, great!

Byron added a commit to Byron/cargo that referenced this issue Mar 16, 2022
…0407)

That way, we know that the case without non-artifact dependencies keeps
working, see rust-lang#10407 (comment)
for the source of the change.
Hezuikn pushed a commit to Hezuikn/cargo that referenced this issue Sep 22, 2022
…0407)

That way, we know that the case without non-artifact dependencies keeps
working, see rust-lang#10407 (comment)
for the source of the change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-bindeps Nightly: binary artifact dependencies
Projects
None yet
Development

No branches or pull requests

3 participants