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

feat(core): deterministic snapshots #14037

Merged
merged 7 commits into from
May 17, 2022
Merged

feat(core): deterministic snapshots #14037

merged 7 commits into from
May 17, 2022

Conversation

littledivy
Copy link
Member

Closes #10783

$ md5sum target/debug/build/deno_runtime-bc62560f5114b792/out/CLI_SNAPSHOT.bin
0205db97c85b0a06d5b0ea6b23258e6f  target/debug/build/deno_runtime-bc62560f5114b792/out/CLI_SNAPSHOT.bin

$ touch ext/console/lib.rs && cargo build
# blabla

$ md5sum target/debug/build/deno_runtime-bc62560f5114b792/out/CLI_SNAPSHOT.bin
0205db97c85b0a06d5b0ea6b23258e6f  target/debug/build/deno_runtime-bc62560f5114b792/out/CLI_SNAPSHOT.bin

bartlomieju
bartlomieju reviewed Mar 20, 2022
View reviewed changes
cli/tests/integration/eval_tests.rs
Comment on lines +23 to +25
// Make sure that snapshot flags don't affect runtime.
#[test]
fn eval_randomness() {
Copy link
Member

@bartlomieju bartlomieju Mar 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can we have a test that checks that snapshots are actually predictable?

Copy link
Member

@ry ry Mar 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'd be happy to hard code the snapshot checksum in the tests (0205db97c85b0a06d5b0ea6b23258e6f)

We just have to have a comment on how to update it easily

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be happy to hard code the snapshot checksum in the tests (0205db97c85b0a06d5b0ea6b23258e6f)

We just have to have a comment on how to update it easily

The snapshot/checksum will change pretty much all the time (slightest JS change in this repo) so that would be impractical IMO

Copy link
Member

Choose a reason for hiding this comment

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

Probably not that often - only if you change the JS code. Many PRs would not update it.

Maybe the hick-up isn't worth it. I feel like we might discover something if we watched the checksum change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a hardcoded checksum test.

@littledivy
Copy link
Member Author

Weird this test doesn't work on the CI but passes locally. https://github.com/denoland/deno/runs/5737674731?check_suite_focus=true#step:32:3144

@bartlomieju
Copy link
Member

It's not really surprising to me that you get different checksum on CI than locally. I'm skeptical about this test being useful at all.

@littledivy littledivy merged commit 68bf43f into denoland:main May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju left review comments

@ry ry ry left review comments

@lucacasonato lucacasonato lucacasonato approved these changes

@AaronO AaronO Awaiting requested review from AaronO

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make snapshots deterministic
5 participants
@littledivy @bartlomieju @ry @AaronO @lucacasonato