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.

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

internal: Switch to expected.assert_eq for ide tests #16252

Merged
merged 7 commits into from
Jan 4, 2024

Conversation

Urhengulas
Copy link
Contributor

@Urhengulas Urhengulas commented Jan 4, 2024

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

  • Can I use Vec? If not, what is the alternative?
    I assume I cannot because of:
    **Architecture Invariant:** rust-analyzer tests do not use libcore or libstd.
  • 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", }
    
  • 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.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 4, 2024
@Veykril
Copy link
Member

Veykril commented Jan 4, 2024

Can I use Vec? If not, what is the alternative?

This is meant for test fixtures (the code that is passed to tests as strings), those dont see std library stuff. Testing infra is free to use whatever.

Should I group it by file, as proposed by Lukas?

On second though thats probably not worth it given the number of files changed is usually 1 anyways

@Urhengulas Urhengulas marked this pull request as ready for review January 4, 2024 13:30
@Veykril
Copy link
Member

Veykril commented Jan 4, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jan 4, 2024

📌 Commit 656ac10 has been approved by Veykril

It is now in the queue for this repository.

@Veykril Veykril changed the title Switch to expected.assert_eq for ide tests internal: Switch to expected.assert_eq for ide tests Jan 4, 2024
@bors
Copy link
Contributor

bors commented Jan 4, 2024

⌛ Testing commit 656ac10 with merge 7f75815...

@bors
Copy link
Contributor

bors commented Jan 4, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 7f75815 to master...

@bors bors merged commit 7f75815 into rust-lang:master Jan 4, 2024
10 checks passed
@Urhengulas Urhengulas deleted the dont-assert-debug branch January 4, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants