Skip to content

Conversation

kkysen
Copy link
Contributor

@kkysen kkysen commented Jul 12, 2022

This is an initial (currently quite hacky) version of PDG snapshot testing for analysis/test. Currently it just shells out to ./scripts/pdg.sh and reads the produced (by c2rust-pdg) analysis/test/pdg.log. I intend to improve this a lot (not shell out, but instead invoke library functions in c2rust_pdg and c2rust_dynamic_instrumentation), but I think it's useful to get a working version running so that we are aware of any PDG changes that occur and can unblock anyone else waiting for this to get more aggressive with changes.

Also, the snapshots/*.snap files need to be committed, right?

@kkysen kkysen requested a review from oinoom July 12, 2022 03:59
@oinoom
Copy link
Contributor

oinoom commented Jul 12, 2022

Also, the snapshots/*.snap files need to be committed, right?

Yes, I believe so.

Copy link
Contributor

@oinoom oinoom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! Great start, this is already a much-needed improvement. I'd love to add this to the pipeline if it's not already there.

@oinoom oinoom merged commit a6fc618 into master Jul 12, 2022
@oinoom oinoom deleted the kkysen/pdg-snapshot-testing branch July 12, 2022 14:49
@kkysen
Copy link
Contributor Author

kkysen commented Jul 12, 2022

@aneksteind, it'll be added to the CI pipeline once #483 lands, as that adds cargo test to CI. If you want to review that PR, that'd be helpful.

@kkysen
Copy link
Contributor Author

kkysen commented Jul 12, 2022

Also, just a note, if the snapshot tests fail, you can review changes with cargo insta review, otherwise cargo test will keep crashing.

@oinoom
Copy link
Contributor

oinoom commented Jul 12, 2022

Also, just a note, if the snapshot tests fail, you can review changes with cargo insta review, otherwise cargo test will keep crashing.

Yep, already making use of this for the new PDG features I'm working on! :)

@kkysen
Copy link
Contributor Author

kkysen commented Jul 12, 2022

Awesome! Yeah, sorry for taking a while. I probably could've implemented this very bare bones version a bit earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants