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

Rework test_plan_serialization_bwc to do roundtrip #7862

Merged
merged 2 commits into from
Jun 8, 2023
Merged

Rework test_plan_serialization_bwc to do roundtrip #7862

merged 2 commits into from
Jun 8, 2023

Conversation

carlopi
Copy link
Contributor

@carlopi carlopi commented Jun 7, 2023 •
edited

Before this PR:

  • "Generate serialized plans file" with GEN_PLAN_STORAGE set generated test/api/serialized_plans/serialized_plans.binary
  • "Generate serialized plans file" without GEN_PLAN_STORAGE did nothing
  • "Test deserialized plans from file" tested against test/api/serialized_plans/serialized_plans.binary

Afterwards

  • "Generate serialized plans file" with GEN_PLAN_STORAGE set generates test/api/serialized_plans/serialized_plans.binary AND test it's deserialization
  • "Generate serialized plans file" without GEN_PLAN_STORAGE generates test/api/serialized_plans/serialized_plans.temp.binary AND test it's deserialization
  • "Test deserialized plans from file" tests against test/api/serialized_plans/serialized_plans.binary

Basically always rountrip test in "Generate serialized plans file" (either on real or temp file) and the same as before in "Test deserialized plans from file".
Also test on generation is run as part of serialization suite.
Main logic should be the same that @bleskes originally committed with minor refactoring.

carlopi
carlopi commented Jun 7, 2023
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
@carlopi carlopi force-pushed the test_plan_serialization branch 2 times, most recently from 452da4e to 2335203 Compare June 7, 2023 12:42
@carlopi
Rework test_plan_serialization_bwc to roundtrip serialization -> dese…
672ebdb 
…rialization

Generation test is now a roundtrip test, either on ignored temporary file OR on
file to be actually committed (if run with GEN_PLAN_STORAGE environment set, either
manually or via `python3 scripts/generate_plan_storage_version.py`
@carlopi
Adapt to feedback from @Mytherin on TestCreatePath
c7b9435 
Now we don't have a temp file hanging anymore
Mytherin
Mytherin approved these changes Jun 8, 2023
View reviewed changes
Copy link
Collaborator

@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@carlopi
Copy link
Contributor Author

carlopi commented Jun 8, 2023

Single CI failure is unrelated: https://github.com/duckdb/duckdb/actions/runs/5208703631/jobs/9399017888?pr=7862#step:6:988

@Mytherin Mytherin merged commit 2714bd3 into duckdb:master Jun 8, 2023
83 of 84 checks passed
@carlopi carlopi deleted the test_plan_serialization branch June 13, 2023 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mytherin Mytherin Mytherin approved these changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@carlopi @Mytherin