detect more failure cases

[Skgland]

I think this is best reviewed commit-by-commit.

The first commit adds a sanity check for the buildtest crates and fixes an existing deviation.

The second commits adds a bunch of buildtest tests and crates that weren't handled before this PR.
The added tests/crates have been found by looking through the crater runs classifier as Error in the crater experiment beta-1.86-1.

The remaining commits work through the added tests to properly classify them.

With these crates classified crater should sort them into the Broken category rather than the Error PrepareFail category. (For the existing PrepareError variants immediately when the dependency is updated, for the new variants they would need to be added to be recognized in detect_broken)

[syphar]

I totally forgot about this PR, generally don't hesitate to ping me if that happens again.

Will try to find some time to do a first review this week.

[syphar]

so, I did a high level check of the code & the tests.

From a pure code perspective it looks good to me.

Thing is: I have not enough cargo & crater knowledge to judge if what you're doing is generally correct.
So I'll reach out to some other people to also validate this.

[Skgland]

It looks like there hasn't been any activity in the Zulip Topic, maybe a ping here would work better? I didn't notice my ping in Zulip until I stumbled over that topic.

[Skgland]

With these crates classified crater should sort them into the Broken category rather than the Error category. (For the existing PrepareError variants immediately when the dependency is updated, for the new variants they would need to be added to be recognized in detect_broken)

After rust-lang/crater#770 this would move them from PrepareFail (instead of Error) to Broken.

[Skgland]

There was a suggestion on Zulip to maybe ask for a cargo reviewer. Has anyone reached out to them?

[Skgland]

Replying to Zulip:

I know the codebase and roughly what it does, the thing missing for me is the cargo error edge cases that they try to catch / differentiate.

So if the given test case actually emits this error, and it can be extracted like in the code

For crater there are two important parts.

1. The error can be attributed to prepare failure vs. build/test failure. ensure callers of BuildDirectory::run can determine if the source of the error is in Prepare::prepare #103 ensures that all prepare failures can be attributed to prepare failure.
2. It can be differentiated between spurious (TestResult::PrepareFail) and non-spurious failure (TestResult::BrokenCrate). This does that by attempting to detect more of the later and categorizing them into a specific error variant (pre-existing were it fits, new one otherwise).

Parsing cargo output is not ideal, but I think its the only way currently available.
All output should be controlled by cargo (no build scripts etc. that could add output are involved),
so I think false positives shouldn't be highly unlikely/impossible.
Using something like --message-format=json would be ideal, but that is not available for all commands and currently cargo errors aren't emitted as json rust-lang/cargo#8283

Regarding the second point if the given test case actually emits this error this is tested in the tests. The test_prepare_error_stderr! macro checks that the specified error message is contained in the stderr String that the PrepareError vriant contains.

While there are two variant (MissingCargoToml and InvalidCargoTomlSyntax) for which a different macro (test_prepare_error!) is used that doesn't check stderr (as those variant don't contain a stderr) all those tests are pre-existing.

[Skgland]

Another note regarding tests:

It's also not ideal that some of the test depend on other git repositories/crates.io crates not under/controlled by rust-lang. I used those because I had trouble recreating them otherwise.

• tests/buildtest/crates/invalid-cargotoml-conflicting-links/Cargo.toml
• tests/buildtest/crates/invalid-cargotoml-content-deps/Cargo.toml
• tests/buildtest/crates/invalid-cargotoml-content-type-in-deps/Cargo.toml
• tests/buildtest/crates/invalid-cargotoml-syntax-deps/Cargo.toml

Edit: build-rs has been transferred to rust-lang so these should actually be fine

• tests/buildtest/crates/invalid-cargotoml-missing-override/Cargo.toml
• tests/buildtest/crates/invalid-cargotoml-missing-patch/Cargo.toml
• tests/buildtest/crates/invalid-cargotoml-missing-registry-config/Cargo.toml
• tests/buildtest/crates/missing-deps-typo/Cargo.toml

[syphar]

After we went through #103 I was wondering how we can make progres on this PR here.

I feel like there is already some coverage where the test-cases in the codebase already cover the error messages you check for in run_command.

Are all possible messages already in test-cases?

[Skgland]

After we went through #103 I was wondering how we can make progress on this PR here.

I feel like there is already some coverage where the test-cases in the codebase already cover the error messages you check for in run_command.

run_command is a combination of the error checking from capture_lockfile and fetch_deps extracted into a separate function so it can be re-used in both.
The reason being that they mostly have the same error cases, if no lockfile exists capture_lockfile is run an should catch the problem, otherwise capture_lockfile is skipped and the problem will be caught in fetch_deps instead. The error handling of those two functions had diverged and as such some errors were only being caught and categorized in one while others only in the other.

Are all possible messages already in test-cases?

For existing checks:

match	capture_lockfile test	fetch_deps test
failed to load source for dependency	N/A	N/A
failed to select a version for the requirement	test_missing_deps_registry_version added in #103	test_yanked_deps
no matching package named	test_missing_deps_registry	N/A

For added checks (all tests added in #103):

match	capture_lockfile test	fetch_deps test
no matching package found	test_missing_deps_typo	N/A
no matching package for override	test_invalid_cargotoml_missing_override	N/A
The patch location does not appear to contain any packages matching the name	test_invalid_cargotoml_missing_patch	N/A
failed to parse manifest at	test_invalid_cargotoml_content_deps	N/A
error: invalid table header	test_invalid_cargotoml_syntax_deps	N/A
error: invalid type:	would need to find a replacement for [email protected] that isn't yanked	test_invalid_cargotoml_content_type_in_deps
error: cyclic feature dependency: feature	test_invalid_cargotoml_cyclic_feature	N/A
error: cyclic package dependency: package	test_invalid_cargotoml_cyclic_package	N/A
error: package collision in the lockfile: packages are different, but only one can be written to lockfile unambiguously	test_lockfile_collision	N/A
error: failed to parse lock file at	skipped when a lockfile exists	test_invalid_lockfile_syntax
error: Attempting to resolve a dependency with more than one crate with links=	N/A	test_invalid_cargotoml_conflicting_links

To turn capture_lockfile tests into fetch_deps tests should be rather simple.
Copy the test crate and add a stale/empty Cargo.lock file. This will cause capture_lockfile to be skipped as a lockfile already exists and the later to have to re-generate the lockfile as it is stale.

For test_invalid_cargotoml_conflicting_links the lockfile was likely just commited by accident,
removing it doesn't appear to change the result of the test.

For test_invalid_cargotoml_content_type_in_deps removing the lockfile changes the test output as cargo refuses to select the yanked dependency, resulting in the error already tested in test_missing_deps_registry_version.