search

rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.78k stars 2.42k forks source link

bindeps: clean up metadata tests #10407

Open ehuss opened 2 years ago

ehuss commented 2 years ago

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

Byron commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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!