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

Hard to review snapshots because they are far from their intended code #10627

Closed
Tracked by #10635
epage opened this issue May 3, 2022 · 1 comment · Fixed by #10638
Closed
Tracked by #10635

Hard to review snapshots because they are far from their intended code #10627

epage opened this issue May 3, 2022 · 1 comment · Fixed by #10638
Labels
A-testing-cargo-itself Area: cargo's tests C-bug Category: bug

Comments

@epage
Copy link
Contributor

epage commented May 3, 2022

Problem

Right now we have

tests/snapshots/add/*
...
tests/testsuite/cargo_add.rs
...

This is mildly annoying to review in small cases but in cases like #10620, its a big pain

Steps

See #10620

Possible Solution(s)

In cargo-test-macro we add a macro that will discover test files in the snapshot directory

To ensure it rebuilds when new tests are added, we add a build.rs that says to rebuild when that directory changes (downside being we'll rebuild more when making test changes which avoiding that was one of the benefits of snapshots)

Notes

We should wait for #10620 so we can update all of them at once

Version

No response
@epage epage added C-bug Category: bug A-testing-cargo-itself Area: cargo's tests labels May 3, 2022
@Muscraft
Copy link
Member

Muscraft commented May 3, 2022 •
edited
Loading

This should also wait for #10626 to be resolved, as it will reorganize snapshot tests

This was referenced May 3, 2022
Closed
Merged
@Muscraft Muscraft mentioned this issue May 4, 2022
Merged
@epage epage mentioned this issue May 5, 2022
Open
7 tasks
bors added a commit that referenced this issue May 6, 2022
@bors
Auto merge of #10631 - Muscraft:move-snapshot-tests, r=epage
acaf8e2 
move one `snapshot/add` test into `testsuite/cargo_add/`

This is an experiment with moving the test code related to a snapshot into the testsuite directory so it's easier to review.

- To kick the tire on these changes, a single test was ported

This is a step towards #10627.  A follow up will port all of the tests

r? `@epage`
@Muscraft Muscraft mentioned this issue May 7, 2022
Merged
@bors bors closed this as completed in 9c61a3d May 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-testing-cargo-itself Area: cargo's tests C-bug Category: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Move snapshot tests into testsuite Muscraft/cargo
2 participants
@epage @Muscraft