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

Replace check_expect test functions that assert by debug equality #14268

Closed
Veykril opened this issue Mar 6, 2023 · 4 comments
Closed

Replace check_expect test functions that assert by debug equality #14268

Veykril opened this issue Mar 6, 2023 · 4 comments
Assignees
@Urhengulas
Labels
A-infra CI and workflow issues C-enhancement Category: enhancement E-unknown It's unclear if the issue is E-hard or E-easy without digging in

Comments

@Veykril
Copy link
Member

Veykril commented Mar 6, 2023 •
edited by rustbot
Loading

We have a bunch of test fixtures which currently assert outputs via the debug representation of things. These are annoying as the debug representation can change in many ways (even by just upgrading the compiler version which did happen in the past). We should get rid of those and replace them with better expected output.

An example of such a function is the following here

rust-analyzer/crates/ide/src/inlay_hints.rs

Lines 541 to 546 in a360fab

#[track_caller]
pub(super) fn check_expect(config: InlayHintsConfig, ra_fixture: &str, expect: Expect) {
let (analysis, file_id) = fixture::file(ra_fixture);
let inlay_hints = analysis.inlay_hints(&config, file_id, None).unwrap();
expect.assert_debug_eq(&inlay_hints)
}

@Veykril Veykril added E-unknown It's unclear if the issue is E-hard or E-easy without digging in A-infra CI and workflow issues C-enhancement Category: enhancement labels Mar 6, 2023
@Urhengulas
Copy link
Contributor

@rustbot assign

@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2024

Error: Parsing assign command in comment failed: ...' assign' | error: specify user to assign to at >| ''...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@Urhengulas
Copy link
Contributor

@rustbot claim

@Urhengulas Urhengulas mentioned this issue Jan 4, 2024
Merged
3 tasks
bors added a commit that referenced this issue Jan 4, 2024
@bors
Auto merge of #16252 - Urhengulas:dont-assert-debug, r=Veykril
7f75815 
internal: Switch to `expected.assert_eq` for `ide` tests

This PR switches from `assert_debug_eq` to `assert_eq` and only compares parts of the result and not the whole. The aim is to only compare parts which are relevant to the test and also make it more readable.

Part of #14268.

## Questions
- [x] Can I use `Vec`? If not, what is the alternative?
    I assume I cannot because of: https://github.com/rust-lang/rust-analyzer/blob/c3a00b5468576de4e39adc8fa5ceae35a0024e49/docs/dev/architecture.md?plain=1#L413
- [x] Should I group it by file, as proposed by Lukas?
    ```
    file_id 1:
        source_file_edits:
            - Indel { insert: "foo2", delete: 4..7 }

    file_id 2:
        file_system_edits:
            MoveFile AnchoredPathBuf { anchor: FileId(2), path: "foo2.rs", }
    ```
- [x] Is it okay to ignore `CreateFile` events? They do not have a FileId, which would be problematic, but they do not occur in the existing tests, so I marked them as `unreachable!()` so far.
This was referenced Jan 4, 2024
Merged
Merged
bors added a commit that referenced this issue Jan 4, 2024
@bors
Auto merge of #16260 - Urhengulas:ide-diagnostics-refactor-tests, r=l…
a795f48 
…nicola

internal: Replace only occurence of `check_expect` with `check_diagnostics`

Part of #14268.
bors added a commit that referenced this issue Jan 5, 2024
@bors
Auto merge of #16256 - Urhengulas:ide-runnables-assert_eq, r=Veykril
e53792b 
internal: Only compare relevant parts in  `ide::{runnables,inlay_hints}` tests

This PR limits the data being compared. Therefore the tests should be more readable, as well as being more robust to changes to the data structure.

Part of #14268.
@Urhengulas
Copy link
Contributor

The three PRs should solve this issue

@Veykril Veykril closed this as completed Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Urhengulas Urhengulas

Labels
A-infra CI and workflow issues C-enhancement Category: enhancement E-unknown It's unclear if the issue is E-hard or E-easy without digging in
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Veykril @Urhengulas @rustbot