Skip to content

Conversation

@kazimuth
Copy link
Contributor

Description of Changes

Plus unit and integration tests.
I'm still working through errors in the integration tests -- I think I'm going to need to something slightly more clever in a few places -- but generally this is ready for review.

API and ABI breaking changes

N/A

Expected complexity level and risk

This is tricky because we're adding a new code path that's supposed to replicate the behavior of another complex code path, but I think once we add the other end (ModuleDef -> TableSchema) comparing the results will be enough to validate that stuff works.

Testing

I test on both synthetic v8 module definitions and actual module definitions from the repo.

@kazimuth kazimuth requested a review from cloutiertyler August 16, 2024 21:35
@bfops bfops added the release-any To be landed in any release window label Aug 19, 2024
@kazimuth kazimuth force-pushed the jgilles/validation branch from 2dba1cf to 058f3d7 Compare August 23, 2024 18:28
@kazimuth kazimuth force-pushed the jgilles/v8validation branch 4 times, most recently from c8465d8 to af07662 Compare August 23, 2024 21:02
@kazimuth kazimuth force-pushed the jgilles/validation branch from e1afd15 to f695d0d Compare August 23, 2024 22:13
@kazimuth kazimuth force-pushed the jgilles/v8validation branch from af07662 to de74b8d Compare August 23, 2024 22:21
@kazimuth kazimuth requested a review from Centril August 23, 2024 23:07
@kazimuth
Copy link
Contributor Author

@Centril @cloutiertyler I consider this ready for merge.

@kazimuth kazimuth force-pushed the jgilles/v8validation branch 2 times, most recently from ab49537 to a8e90e1 Compare August 26, 2024 18:03
@kazimuth kazimuth force-pushed the jgilles/v8validation branch from a8e90e1 to 06a9e70 Compare August 27, 2024 19:00
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

In principle this all seems fine to me.

@kazimuth kazimuth changed the base branch from jgilles/validation to master August 28, 2024 16:26
Whoops, that didn't need to be there

Placate clippy

Paragraph comments

Break tests, then fix them

Address review comments + fix clippy

Update to deal with accessor_name

Fix import

Update for removed structural_ty

Add lifecycle validation

Validate scheduled reducers, whoops
@kazimuth kazimuth force-pushed the jgilles/v8validation branch from 3ad5756 to 8834292 Compare August 28, 2024 16:30
@kazimuth kazimuth added this pull request to the merge queue Aug 29, 2024
Merged via the queue into master with commit 98faf7d Aug 29, 2024
@kazimuth kazimuth deleted the jgilles/v8validation branch August 29, 2024 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-any To be landed in any release window

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants