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.

Sign up for GitHub

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

Jump to bottom

Test rust-analyzer in CI #99444

Closed
wants to merge 2 commits into from
Closed

Test rust-analyzer in CI #99444

wants to merge 2 commits into from

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Jul 19, 2022

This adds x.py support for testing rust-analyzer in-tree, and runs
that in CI for x86_64-gnu-tools so we can track toolstate.

@cuviper
Add ./x.py test rust-analyzer
7537def 
This adds support for testing `rust-analyzer` in-tree.
@cuviper
Test rust-analyzer in CI
b26e897
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jul 19, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2022
@cuviper
Copy link
Member Author

cuviper commented Jul 19, 2022

However, the tests are failing locally for me in-tree, and I don't know why. I'm aware of the proc-macro bridge issues due to nightly changes, but even with cargo +stable test I get a lot of failures where "Expect:" is empty. The same commit tests fine for me in an independent checkout of the rust-analyzer repo.

@cuviper cuviper mentioned this pull request Jul 19, 2022
Closed
@cuviper
Copy link
Member Author

cuviper commented Jul 19, 2022

If the whole testsuite is infeasible, we could also narrow it -- e.g. -p proc-macro-srv shows the current problems:

failures:
    tests::test_attr_macro
    tests::test_derive_empty
    tests::test_derive_error
    tests::test_fn_like_macro
    tests::test_fn_like_macro2

@fasterthanlime fasterthanlime mentioned this pull request Jul 19, 2022
Closed
@fasterthanlime
Copy link
Contributor

even with cargo +stable test I get a lot of failures where "Expect:" is empty

I'm able to reproduce the failures when checking out your branch. Those tests use expect-test's expect_file!.

strace tells the story:

$ clear; strace -e open,openat -ff cargo t test_string_highlighting
(lots of output omitted)
[pid 38722] openat(AT_FDCWD, "/home/amos/bearcove/rust/crates/ide/src/syntax_highlighting/./test_data/highlight_strings.html", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)

Indeed that file doesn't exist:

$ stat /home/amos/bearcove/rust/crates/ide/src/syntax_highlighting/./test_data/highlight_strings.html
stat: cannot statx '/home/amos/bearcove/rust/crates/ide/src/syntax_highlighting/./test_data/highlight_strings.html': No such file or directory

The correct path is as follows:

$ stat /home/amos/bearcove/rust/src/tools/rust-analyzer/crates/ide/src/syntax_highlighting/./test_data/highlight_strings.html 
  File: /home/amos/bearcove/rust/src/tools/rust-analyzer/crates/ide/src/syntax_highlighting/./test_data/highlight_strings.html
  Size: 45127           Blocks: 96         IO Block: 4096   regular file
Device: 803h/2051d      Inode: 16409270    Links: 1
Access: (0664/-rw-rw-r--)  Uid: ( 1000/    amos)   Gid: ( 1000/    amos)
Access: 2022-07-18 13:45:36.999588514 +0200
Modify: 2022-07-18 13:44:44.295860084 +0200
Change: 2022-07-18 13:44:44.295860084 +0200
 Birth: 2022-07-18 13:44:44.295860084 +0200

That feels like a bug in expect-test. It uses the file! macro then later uses it to build an absolute path by finding the workspace root. However, it goes too deep and finds rust's top-level Cargo.toml, and then the paths are all wrong.

This was referenced Jul 19, 2022
Closed
Merged
fasterthanlime added a commit to fasterthanlime/rust-analyzer that referenced this pull request Jul 19, 2022
@fasterthanlime
Upgrade to expect-test@1.4.0
1b41647 
cf. rust-analyzer/expect-test#33
cf. rust-lang/rust#99444 (comment)
@fasterthanlime fasterthanlime mentioned this pull request Jul 19, 2022
Merged
bors added a commit to rust-lang/rust-analyzer that referenced this pull request Jul 19, 2022
@bors
Auto merge of #12817 - fasterthanlime:expect-test-1-4-0, r=lnicola
7d20ff3 
Upgrade to expect-test@1.4.0, add CARGO_WORKSPACE_DIR env var

This should make ra's test suite runnable from within `rust-analyzer/rust`.

`@cuviper` ran into that when trying to run RA tests from rust CI: rust-lang/rust#99444 (comment)
fasterthanlime added a commit to fasterthanlime/rust that referenced this pull request Jul 19, 2022
@fasterthanlime
:up_arrow: rust-analyzer
4f37325 
This bumps `expect-test` to 1.4.0, which is required for tests to
pass from the `rust-lang/rust` repo

cf. rust-lang#99444
@fasterthanlime fasterthanlime mentioned this pull request Jul 19, 2022
Closed
@fasterthanlime
Copy link
Contributor

@cuviper I opened a PR against your PR to also bump the rust-analyzer submodule, which should let tests pass.

@fasterthanlime
Copy link
Contributor

@cuviper I opened a PR against your PR to also bump the rust-analyzer submodule, which should let tests pass.

So, this is enough to make cargo check pass from rust/src/tools/rust-analyzer, but not from x.py test rust-analyzer. I guess the .cargo/config.toml doesn't get picked up then?

The alternative there is to set CARGO_WORKSPACE_DIR from x.py directly. (The changes to expect-test are still good since we still want to be able to run cargo check from within the rust-analyzer tree in rust-lang/rust)

fasterthanlime added a commit to fasterthanlime/rust that referenced this pull request Jul 19, 2022
@fasterthanlime
Set CARGO_WORKSPACE_DIR directly in prepare_tool_cargo
9c766ad 
cf. rust-lang#99444 (comment)

cf. rust-analyzer/expect-test#33
@fasterthanlime
Copy link
Contributor

I opened a PR against your PR

I updated this PR to set CARGO_WORKSPACE_DIR from "x.py" (well, really, bootstrap::tool::prepare_tool_cargo) and all rust-analyzer tests now pass with ./x.py test rust-analyzer 🎉

@fasterthanlime fasterthanlime mentioned this pull request Jul 19, 2022
Closed
@fasterthanlime
Copy link
Contributor

I opened #99465 for research purposes - it re-adds rust-analyzer as a subtree, and it includes all commits from this here PR & my follow-up.

The reason I did this now is because @jyn514 and others suggested "skipping toolstate altogether" and going straight for subtree.

fasterthanlime added a commit to fasterthanlime/rust that referenced this pull request Jul 19, 2022
@fasterthanlime
Add ./x.py test rust-analyzer
0b33563 
This adds support for testing `rust-analyzer` in-tree.

Test rust-analyzer in CI

Set CARGO_WORKSPACE_DIR directly in `prepare_tool_cargo`

cf. rust-lang#99444 (comment)

cf. rust-analyzer/expect-test#33

Various x.py changes for Submodule => InTree, and removing toolstate

Introduce `rust-analyzer/in-rust-tree` cargo feature

This allows skipping ra's "check_merge_commits" test for now.

Later, it might be used to link against `extern proc_macro` directly,
cf. rust-lang/rust-analyzer#12803

More cleanups around the RustAnalyzer tool in bootstrap

Start fixing lints

Deny the same warnings x.py does in all RA crates

Warn on x.py warnings, don't deny

cf. https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Frust-analyzer/topic/rust-analyzer.20as.20a.20subtree.3A.20experimental.20PR.20status/near/290114101

Fix rust_2018_idioms with cargo fix

Add warning groups for more RA crates

Fix more 2015 idioms (with 2018_idioms, naming is fun)

RA passes all x.py warnings

Fix formatting after fixing idioms/additional warnings

Enable in-rust-tree feature, RA tests pass

Print stdout+stderr when nested cargo invocation fails
@Mark-Simulacrum
Copy link
Member

Thanks! Closing per https://rust-lang.zulipchat.com/#narrow/stream/301329-t-devtools/topic/test.20--stage.200.20rust-analyzer/near/290636735.

Veykril pushed a commit to Veykril/rust-analyzer that referenced this pull request Aug 9, 2022
@fasterthanlime @Veykril
Upgrade to expect-test@1.4.0
6220d84 
cf. rust-analyzer/expect-test#33
cf. rust-lang/rust#99444 (comment)
@cuviper cuviper deleted the test-rust-analyzer branch October 15, 2022 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@cuviper @rust-highfive @fasterthanlime @Mark-Simulacrum @rustbot