cargo icon indicating copy to clipboard operation
cargo copied to clipboard

WIP: Add normalized querykind Fixes #1079

Open tomharmon opened this issue 3 years ago • 4 comments

What does this PR try to resolve?

See this thread, particularly this comment

How should we test and review this PR?

Currently trying to get a functional or ui test to work but am having issues.

If you don't know how to write and run your tests, please read the guide: https://doc.crates.io/contrib/tests

Additional information

TODO:

  • tests
    • UI Test: When I tried to write this test, I was having issues replicating the issue since I couldn't get the .cargo/config.toml file in tests/testsuite/<command>/<case>/in to be persisted. I have this test on another branch, add-normalized-querykind-test, which is just master + tests.
    • Functional test, this is the output:
Click to expand
 ~/code/cargo/ [add-normalized-querykind+] cargo  test --test testsuite -- cargo_add_with_vendored_pkgs
> cargo test --test testsuite -- cargo_add_with_vendored_pkgs
   Compiling cargo v0.65.0 (/Users/thomasharmon/code/cargo)
    Finished test [unoptimized + debuginfo] target(s) in 3.64s
     Running tests/testsuite/main.rs (target/debug/deps/testsuite-a0b7d1f599e8fa04)

running 1 test
test cargo_add_with_vendored_pkgs::carg_add_with_vendored_packages ... FAILED

failures:

---- cargo_add_with_vendored_pkgs::carg_add_with_vendored_packages stdout ----
running `/Users/thomasharmon/code/cargo/target/debug/cargo vendor ./vendor`
running `/Users/thomasharmon/code/cargo/target/debug/cargo add cbindgen`
thread 'cargo_add_with_vendored_pkgs::carg_add_with_vendored_packages' panicked at '
test failed running `/Users/thomasharmon/code/cargo/target/debug/cargo add cbindgen`
error: stdout did not match:
1        -    Updating crates.io index
2        -      Adding xml-rs v0.8.4 to dependencies.


other output:
warning: translating `cbindgen` to `xml-rs`
      Adding xml-rs v0.8.4 to dependencies.

', tests/testsuite/cargo_add_with_vendored_pkgs.rs:49:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    cargo_add_with_vendored_pkgs::carg_add_with_vendored_packages

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2597 filtered out; finished in 29.65s

error: test failed, to rerun pass '--test testsuite'

tomharmon avatar Aug 13 '22 22:08 tomharmon

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

rust-highfive avatar Aug 13 '22 22:08 rust-highfive

I didn't look into the issue. Just remind that the test seems to perform real network requests. You may take existing tests as reference for setting up a fake registry and dependencies.

weihanglo avatar Aug 14 '22 19:08 weihanglo

You're definitely right, I updated my test to reflect that. However I'm still having some issues which I need to debug in under to get a better understand to be able to ask the right questions. The test now replicates the original error and makes an assertion of what the correct output should be. It's still not passing though.

Right now I can setup a debugger in VSCode but don't know how to attach it to the cargo process itself so I can debug what's actually happening in the test (hope that makes sense). I know how to do it with CLion I think, so I'm going to try to buy the license for that so I can get the debugger going.

Thanks for the comment! I'll be back with a better understanding after I get the debugger running and attaching it to the actual cargo add process I'm testing, and not the test itself

tomharmon avatar Aug 15 '22 14:08 tomharmon

Thank you @epage extensively for your extremely generous review and welcome in Zulip. I'll look into these things today and fix them up. This is awesome. I'm so excited to be learning and contributing to cargo so thanks so much for giving me some of your time. It's immensely helpful and appreciated. Cheers.

tomharmon avatar Aug 15 '22 14:08 tomharmon

Ping @goodSyntax808. Checking in to see if you still got time to sort this out, or if you had any questions. Feel free to continue the discussion here or on Zuilp.

weihanglo avatar Feb 09 '23 11:02 weihanglo

:umbrella: The latest upstream changes (presumably #11937) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Apr 06 '23 17:04 bors