test: add derive macro tests #514

r3stl355 · 2024-11-20T12:11:34Z

What changes are proposed in this pull request?

A simplified version of PR429 (to be closed/abandoned) using a doctest trick.

I had to make a small change to the macro code itself, changing crate:: todelta_kernel:: in the generated code.
Used existing developer-visibility feature instead of adding a new one as it seemed to fit the purpose
Added an extra command to justfile to run the new tests as part of existing testing process

How was this change tested?

All tests pass, including the newly added

Signed-off-by: Niko <[email protected]>

codecov · 2024-11-20T15:13:09Z

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.28%. Comparing base (f67c1cc) to head (b2b82b7).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
derive-macros/src/lib.rs	66.66%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #514   +/-   ##
=======================================
  Coverage   83.28%   83.28%           
=======================================
  Files          93       93           
  Lines       22384    22384           
  Branches    22384    22384           
=======================================
  Hits        18642    18642           
  Misses       2786     2786           
  Partials      956      956

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

kernel/src/lib.rs

justfile

kernel/src/lib.rs

scovich

LGTM

kernel/src/lib.rs

derive-macros/src/lib.rs

zachschuermann

left a few questions!

derive-macros/src/lib.rs

kernel/src/lib.rs

kernel/src/actions/mod.rs

kernel/src/lib.rs

r3stl355 · 2024-11-25T20:10:09Z

I added a second part that fixes an error in macro when fully-qulified multi-part types are used (forgot to push this earlier). Also added a passing docs test to verify, this test needs to be moved to normal test as part of #526

scovich

LGTM, thanks!

zachschuermann

hey @r3stl355 are you able to pick this back up? just added a quick comment and would love to have this land :)

kernel/src/lib.rs

Signed-off-by: Niko <[email protected]>

r3stl355 · 2025-04-25T14:06:22Z

Made a bit of a mess and had to force-push, hope that's alright. Should be good now

scovich

Very nice! Just one simplification left and should be ready for merge

kernel/src/actions/mod.rs

Signed-off-by: Niko <[email protected]>

scovich · 2025-04-26T20:28:07Z

Hmm @zachschuermann it looks like something went wrong with the doctest+internal API combo? I thought we had set the CI to enable internal API while compiling doctests?

r3stl355 · 2025-04-26T21:06:15Z

Also, strangely, if I run just cargo test --doc on my machine without specifying any features the tests pass.

## What changes are proposed in this pull request? With the introduction of `EvaluationHandler::create_one`, we can create `EngineData` now. This PR adds a handy new procedural macro to implement a new `IntoEngineData` trait which allows for `self.into_engine_data(schema, &dyn Engine)` as long as all fields implement `Into<Scalar>` (for now just support the simple case of all-primitive fields) This would typically be used in conjunction with `ToSchema` to provide the schema for the output `EngineData`. ### Future work Need to implement tests (make doctest on `IntoEngineData` trait actually build): lump that in to #514 1. need to rename `crate::` to `delta_kernel::` in macros 2. need to make all the types we need conditionally pub (that is, if `cfg(doctest)` then make stuff `pub`) 3. need to do an `extern crate self as delta_kernel` 4. and finally, make the doctest a `no_run` (building) doctest ## How was this change tested? new UT

r3stl355 · 2025-06-05T13:34:54Z

@zachschuermann where does this one sit with all the recent changes, is it still relevant? I am leaving Databricks so need to keep myself occupied :)

scovich · 2025-06-06T12:09:10Z

where does this one sit with all the recent changes, is it still relevant?

AFAIK, this is still important. In particular, it adds the ability to create working doctests for derive macros, which other recent PR would benefit from. I'll let @zachschuermann weigh in on whether/how this PR may need to expand tho.

zachschuermann · 2025-06-06T17:05:52Z

Hi @r3stl355! Yes this is even more relevant after #830 landed :) - see the PR description over there for some overlapping derive macro test work that we need.

I opened another issue that basically just links to this PR and my PR: #991

I believe the remaining work would just be to (1) update this PR to work with the original stuff it intended to test and (2) add to this or open a new PR to expand the testing to my new #[derive(IntoEngineData)] macro!

r3stl355 · 2025-06-06T18:18:37Z

Cool, thanks @scovich & @zachschuermann, I'll see what I can come up with in the next few days

Signed-off-by: Niko Ulmasov <[email protected]>

r3stl355 · 2025-07-16T17:52:28Z

Hi @zachschuermann, I updated the PR to work with latest changes. I suggest creating a different one for #[derive(IntoEngineData)] macro, otherwise I'll never get this one over the fence :)

r3stl355 · 2025-07-18T20:14:22Z

Hey, these failed checks look unrelated to my changes, is there something I need to do to pass them?

zachschuermann · 2025-07-18T20:35:28Z

ah yep just need to rebase on current master and should be good to go! I just updated here via github UI and will take a look at this today :) thank you @r3stl355!

kernel/src/schema/derive_macro_utils.rs

Signed-off-by: Niko Ulmasov <[email protected]>

zachschuermann · 2025-07-22T15:47:35Z

thank you @r3stl355!

r3stl355 · 2025-07-22T16:52:16Z

Thank you for merging @zachschuermann, I'll start looking into the other macro tests

add derive macro tests

309c23e

Signed-off-by: Niko <[email protected]>

github-actions bot assigned r3stl355 Nov 20, 2024

scovich requested review from nicklan and scovich November 20, 2024 15:08

scovich reviewed Nov 20, 2024

View reviewed changes

r3stl355 mentioned this pull request Nov 22, 2024

Add tests to verify schema generated by derive macro #526

Open

Merge branch 'main' into macro-tests

9034cf5

scovich approved these changes Nov 23, 2024

View reviewed changes

kernel/src/lib.rs Outdated Show resolved Hide resolved

derive-macros/src/lib.rs Show resolved Hide resolved

zachschuermann reviewed Nov 25, 2024

View reviewed changes

derive-macros/src/lib.rs Show resolved Hide resolved

kernel/src/lib.rs Show resolved Hide resolved

kernel/src/actions/mod.rs Outdated Show resolved Hide resolved

kernel/src/lib.rs Outdated Show resolved Hide resolved

zachschuermann changed the title ~~[chore] add derive macro tests~~ [test] add derive macro tests Nov 25, 2024

scovich approved these changes Nov 25, 2024

View reviewed changes

zachschuermann changed the title ~~[test] add derive macro tests~~ test: add derive macro tests Mar 19, 2025

zachschuermann approved these changes Mar 28, 2025

View reviewed changes

kernel/src/lib.rs Show resolved Hide resolved

r3stl355 force-pushed the macro-tests branch from 3a36c45 to de93dbe Compare April 25, 2025 12:23

Merge latest 'main' and update to work with changes

0bf673e

Signed-off-by: Niko <[email protected]>

r3stl355 force-pushed the macro-tests branch from de93dbe to 0bf673e Compare April 25, 2025 14:02

Update justfile

2d6c8e4

Signed-off-by: Niko <[email protected]>

scovich approved these changes Apr 25, 2025

View reviewed changes

kernel/src/actions/mod.rs Outdated Show resolved Hide resolved

Change from old way to handling internal apis to new style

a7a8668

Signed-off-by: Niko <[email protected]>

This was referenced May 22, 2025

feat: new #[derive(IntoEngineData)] proc macro #830

Merged

derive macro tests #991

Open

r3stl355 added 3 commits July 15, 2025 21:45

Merge branch 'main' into macro-tests

1de89fb

Merge branch 'main' into macro-tests

85bde6b

Update to match recent changes to other code

b37c45c

Signed-off-by: Niko Ulmasov <[email protected]>

zachschuermann reviewed Jul 18, 2025

View reviewed changes

kernel/src/schema/derive_macro_utils.rs Outdated Show resolved Hide resolved

r3stl355 added 2 commits July 19, 2025 18:49

Merge branch 'main' into macro-tests

2a0d61f

switch to using internal_api macro

fcba246

Signed-off-by: Niko Ulmasov <[email protected]>

r3stl355 force-pushed the macro-tests branch from 1a9517e to fcba246 Compare July 19, 2025 18:27

Merge branch 'main' into macro-tests

b2b82b7

zachschuermann merged commit 245c10c into delta-io:main Jul 22, 2025
20 of 21 checks passed

Uh oh!

test: add derive macro tests #514

test: add derive macro tests #514

Uh oh!

Conversation

r3stl355 commented Nov 20, 2024

What changes are proposed in this pull request?

How was this change tested?

Uh oh!

codecov bot commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

r3stl355 commented Nov 25, 2024

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

r3stl355 commented Apr 25, 2025

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scovich commented Apr 26, 2025

Uh oh!

r3stl355 commented Apr 26, 2025

Uh oh!

r3stl355 commented Jun 5, 2025

Uh oh!

scovich commented Jun 6, 2025

Uh oh!

zachschuermann commented Jun 6, 2025

Uh oh!

r3stl355 commented Jun 6, 2025

Uh oh!

r3stl355 commented Jul 16, 2025

Uh oh!

r3stl355 commented Jul 18, 2025

Uh oh!

zachschuermann commented Jul 18, 2025

Uh oh!

Uh oh!

Uh oh!

zachschuermann commented Jul 22, 2025

Uh oh!

r3stl355 commented Jul 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 20, 2024 •

edited

Loading