Add artifact kind to binary_id if it's not 'lib'#76
Conversation
It's possible for a package to have more than one target with the same name as the package, such as with lib+bin crates. This change adds the target kind to the binary_id in cases where the kind is not lib to ensure binary_id is unique.
| } else if !artifact.target.kind.contains(&"lib".to_owned()) { | ||
| if let Some(kind) = artifact.target.kind.get(0) { |
There was a problem hiding this comment.
Based on https://doc.rust-lang.org/cargo/commands/cargo-metadata.html it seems like kind will only ever have one item in it unless it's a lib. I don't have any experience working with cargo metadata, though, so I'm not sure this is the right approach.
This also means if there's nothing in the kind vec, it will silently use the package name for the binary_id - should this be an error instead?
There was a problem hiding this comment.
Nothing in the kind vec is definitely weird and should at least be a warning I think.
sunshowers
left a comment
sunshowers
left a comment
There was a problem hiding this comment.
Thank you for catching this and for the PR with the excellent description!
| } else if !artifact.target.kind.contains(&"lib".to_owned()) { | ||
| if let Some(kind) = artifact.target.kind.get(0) { |
There was a problem hiding this comment.
Nothing in the kind vec is definitely weird and should at least be a warning I think.
| if artifact.target.name != package.name() { | ||
| binary_id.push_str("::"); | ||
| binary_id.push_str(&artifact.target.name); |
There was a problem hiding this comment.
Hmm, what happens if there's multiple bin targets within a crate, each with their own tests?
I think the best scheme for this is probably something like:
<crate-name> for the lib crate (unit tests)
<crate-name>::<bin-name> for test targets
<crate-name>::<bin/example>/<bin-name> for other targets, e.g in the included test, nextest-tests::bin/nextest-tests
What do you think? Happy to hear suggestions as well.
(And could you add more tests/examples around this? Thanks!)
There was a problem hiding this comment.
Aha, you're right - the problem is more general than I thought. I think your naming scheme makes sense - I'll give it a go.
There was a problem hiding this comment.
So I think this works, with one small exception: if the name of the lib is different from the package, it's not reflected in the binary_id now. It's still unique and un-ambiguous, but we could also use
<crate-name>::lib/<lib-name> in that case if you think it'd be confusing not seeing it.
There was a problem hiding this comment.
if the name of the lib is different from the package, it's not reflected in the binary_id now.
Good catch, I keep forgetting about this situation. I wouldn't address this personally, I think the unambiguous crate name is good enough.
|
Thank you! Will let a test run finish and then land this. |
|
Could you update the PR for the test failures? We do support Rust 1.54 which doesn't support f-strings. |
Binaries within a package are allowed to have the same name so long as they are of different types. This changes the way binary_ids are generated to take this into account and ensure binary_ids are unique.
pmsanford
force-pushed
the
make-binary-id-unique
branch
from
fcf20d4 to
6a4e550
Compare
|
Woops! Caught that in one file but missed it in the error impl |
|
Thanks again! |
2 participants
In a crate with multiple targets that have the same name as the package, if both have tests, tests from one of the targets will be missing from the JSON output. I ran into this is in a lib + bin package. You can see a minimal example here.
This is because of the way
RustTestArtifact.binary_idis constructed. Currently, if the target name is equal to the package name, the package name is taken as thebinary_id. There are cases (such as lib + bin) where two targets will have the same name as the package name. This results inbinary_idbeing non-unique in those cases, and the doc comment forbinary_idsays it should be unique. Currently this only seems to affect the JSON output of the list command - the human readable output of list is still correct and all the tests are still run.The approach this PR takes is, in cases where the target name is equal to the package name, check the target type. If it's anything other than lib, append the type to the name to build the binary ID. I also considered fixing this at the point where the summaries are built for serialization, but it seems like
binary_idis truly intended to uniquely identify a test suite, so I thought this was a more appropriate place.