test: snapshot testing for stream executors #9787

xxchan · 2023-05-14T16:48:35Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Introduce snapshot testing to make writing executor tests easier. See the doc in src/stream/tests/it/snapshot.rs for more details.

Checklist For Contributors

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
~~[ ] I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).~~
I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
All checks passed in ./risedev check (or alias, ./risedev c)

Checklist For Reviewers

I have requested macro/micro-benchmarks as this PR can affect performance substantially, and the results are shown.

Documentation

My PR DOES NOT contain user-facing changes.

Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

Installation and deployment
Connector (sources & sinks)
SQL commands, functions, and operators
RisingWave cluster configuration changes
Other (please specify in the release note below)

Release note

...ver_window/snapshots/risingwave_stream__executor__over_window__eowc__tests__over_window.snap

xxchan · 2023-05-14T16:52:37Z

PTAL and tell whether you like this idea.

st1page · 2023-05-14T17:20:38Z

how about splitting src/stream/src/ and src/stream/src/test? In other words, the test in this PR will be in src/stream/src/test/executor/over_window. This arrangement of test may not be able to access some private field, but currently most of our test does not acquire access them at all.

Btw, I remember we have mentioned that we can move the unit test into a separate because our streaming executor's tests do not need access private field and methods. And after that we can depend on more utils such as frontend crate. But I can not find that comments or issue.

stdrc · 2023-05-15T03:41:41Z

Btw, I remember we have mentioned that we can move the unit test into a separate because our streaming executor's tests do not need access private field and methods. And after that we can depend on more utils such as frontend crate. But I can not find that comments or issue.

Not sure whether it's this comment: #7881 (comment)

BugenZhao

I'm not sure whether there'll be many executors that can benefit from this. 🤔

Some tests are directly manipulating the internal states.
Some tests require more complicated control flows. (

risingwave/src/stream/src/executor/merge.rs

Line 553 in bd23cc0

async fn test_configuration_change() {

)

Actually, executor unit tests are not that "snapshot testing" to me. 🤔 The output is supposed to be manually derived by the developer and can be rarely changed. The ideas of simplifying these tests with DSL look good to me though.

src/stream/src/executor/over_window/eowc.rs

xxchan · 2023-05-15T13:04:56Z

I'm not sure whether there'll be many executors that can benefit from this.

I also have similar concerns so created a demo to gain feedbacks :p

I guess most executor tests are simple enough to be tested in this way. The only exceptions in my mind are:

binary executors (i.e., JOIN)
"control" executors

Some tests are directly manipulating the internal states / require more complicated control flows

Yes, so we don't use snapshot testing for such more fine-grained tests.

Actually, executor unit tests are not that "snapshot testing" to me. 🤔 The output is supposed to be manually derived by the developer and can be rarely changed. The ideas of simplifying these tests with DSL look good to me though.

The real motivation (as discussed with @TennyZhuang) is that manually write these tests are too tiring and the test coverage is low!!! 😄

Mainly for adding new tests, instead of replacing current tests
Generating outputs is good, and avoiding boilerplates is good.

So we may hope to find a way to write the simple test pattern (overlapping input/output events) easier. The granularity is between e2e and complete unit tests. It's kind of 1-executor slt where we manipulate messages directly (I'm not sure whether it's possible and if so, whether it's easier to use slt for these tests).

wangrunji0408 · 2023-05-16T06:35:09Z

I have thought about such test script and it looks pretty cool!
I'm just curious why it is called "snapshot" testing, instead of "executor" testing?

stdrc · 2023-05-16T06:49:13Z

I'm just curious why it is called "snapshot" testing, instead of "executor" testing?

I agree with the name, it's comparing the output snapshot of a correct (committed) version of executor with latest one.

The idea looks great to me! Follow the same idea, can we also make e2e tests snapshot testing (automatically generate output)?

xxchan · 2023-05-16T08:02:09Z

I'm just curious why it is called "snapshot" testing, instead of "executor" testing?

I agree with the name, it's comparing the output snapshot of a correct (committed) version of executor with latest one.

Yes, that's true. To my understanding, "snapshot testing" is basically "file-based tests". The main idea is just commit the entire output!.

It's also called "golden tests" (and the files are called golden files). Here's a blog introducing it https://www.cs.cornell.edu/~asampson/blog/turnt.html

e2e tests snapshot testing (automatically generate output)?

I also have the same question but not sure. Sqllogictest and planner test both support auto completion, so to me they are already kind of "snapshot testing". The only difference is that they put input/output in the same file. Actually that's desired, but we can still do so (contain input in the generated output) for snapshots, which is already done in this PR.

Technically I think that's doable (and not hard). The only reason not to do it I can come up with is that current situation is good enough. (I think for sqllogictest that's true, but for planner tests we have some pain points and had some discussions about it #8557)

soundOfDestiny

LGTM. thanks!

.../snapshots/risingwave_stream__executor__over_window__eowc__tests__over_window_aggregate.snap

kwannoel

I think this will be very useful to improve test coverage. More importantly it can help us locate bug sources at the executor level. Currently I rely on e2e sql to test executor, but that is not ergonomic to use when we want test something specific for the executor.

Generally LGTM, further improvements can be made in subsequent PRs.

TennyZhuang · 2023-05-17T07:30:15Z

It's exactly what I want, LGTM.

We can replace the current tests step by step.

codecov · 2023-05-17T09:54:28Z

Codecov Report

Merging #9787 (9c6cd87) into main (539b061) will decrease coverage by 0.04%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main    #9787      +/-   ##
==========================================
- Coverage   71.14%   71.11%   -0.04%     
==========================================
  Files        1250     1250              
  Lines      209398   209198     -200     
==========================================
- Hits       148970   148761     -209     
- Misses      60428    60437       +9

Flag	Coverage Δ
rust	`71.11% <75.00%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/stream/src/executor/mod.rs	`50.98% <ø> (ø)`
src/stream/src/executor/over_window/eowc.rs	`92.21% <ø> (-2.66%)`	⬇️
src/stream/src/executor/project_set.rs	`68.88% <ø> (-17.52%)`	⬇️
src/stream/src/executor/test_utils.rs	`91.27% <ø> (ø)`
src/common/src/array/stream_chunk.rs	`85.61% <75.00%> (-0.15%)`	⬇️

... and 8 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

stdrc · 2023-05-17T22:59:48Z

This inspired me to come up with another idea: Just between "fully DSL" and "fully hand-written", a combination of both might be interesting: we "run_until_pending" and then print the output in the DSL form (which can be automatically generated).

The idea is interesting! But what if later someday we want to add mock state store that is not in-memory? Then the output is not available at the first pending.

BugenZhao · 2023-05-18T02:12:40Z

calling next().await.unwrap().unwrap() repeatedly is mostly just noise for reviewers and annoying for writers.

IIRC, we have ExecutorTestExt for simplifying this. 🥰

xxchan · 2023-05-18T10:19:51Z

calling next().await.unwrap().unwrap() repeatedly is mostly just noise for reviewers and annoying for writers.

IIRC, we have ExecutorTestExt for simplifying this. 🥰

I know that. But what I like most is replacing multiple expect into one large one. e.g., take a look at the changes of TopN. How do you think?

xxchan · 2023-05-18T15:29:59Z

My final decisions:

all-in expect_test, because the workflow is simpler and it's good enough.
use inline output, because it might be better to review.
keep both of the styles (code input and DSL script input), because I can't tell which is better.
put the new style only in /tests/it to force you write new tests there... (Consider using integration test instead of unit test #9878)

Not decided:

check_until_pending or check_n_steps

This PR is ready to merge. Final comments welcomed.

stdrc

So is expect![[]] auto-updated on risedev test? What if developer forgets to run risedev test locally, will CI check that?

stdrc · 2023-05-19T03:31:57Z

src/stream/tests/it/eowc.rs

+    check_with_script(
+        || create_executor(calls.clone(), store.clone()),
+        r###"
+- !barrier 1
+- !chunk |2
+      I T  I   i
+    + 1 p1 100 10
+    + 1 p1 101 16
+    + 4 p2 200 20
+- !chunk |2
+      I T  I   i
+    + 5 p1 102 18
+    + 7 p2 201 22
+    + 8 p3 300 33
+# NOTE: no watermark message here, since watermark(1) was already received
+- !barrier 2
+- recovery
+- !barrier 3
+- !chunk |2
+      I  T  I   i
+    + 10 p1 103 13
+    + 12 p2 202 28
+    + 13 p3 301 39
+- !barrier 4
+"###,


This looks a little bit ugly. Can we add indent here?

Can we add indent here?

Yes

This looks a little bit ugly.

I don't know. In theory this is a yaml, so somebody may argue that no indent make more sense. I have no preference 😇

src/stream/tests/it/main.rs

xxchan · 2023-05-19T07:19:26Z

So is expect![[]] auto-updated on risedev test? What if developer forgets to run risedev test locally, will CI check that?

It's only updated if the envvar is set. It's just like a normal assert_eq otherwise (with diff in the error message). And there will also be a hint in the error message. See https://docs.rs/expect-test/latest/expect_test/ for more info

stdrc · 2023-05-19T08:02:33Z

So is expect![[]] auto-updated on risedev test? What if developer forgets to run risedev test locally, will CI check that?

It's only updated if the envvar is set. It's just like a normal assert_eq otherwise (with diff in the error message). And there will also be a hint in the error message. See docs.rs/expect-test/latest/expect_test for more info

Will be nice to have sth like risedev update-integration-tests😁

xxchan · 2023-05-19T08:40:41Z

Will be nice to have sth like risedev update-integration-tests😁

I don't want to add a script for such simple task and feel env var is good enough. But I don't object to adding it neither. 🤪

test: snapshot testing for stream executors

a39d6f3

github-actions bot added the component/test Test related issue. label May 14, 2023

xxchan commented May 14, 2023

View reviewed changes

...ver_window/snapshots/risingwave_stream__executor__over_window__eowc__tests__over_window.snap Outdated Show resolved Hide resolved

xxchan commented May 14, 2023

View reviewed changes

...ver_window/snapshots/risingwave_stream__executor__over_window__eowc__tests__over_window.snap Outdated Show resolved Hide resolved

xxchan requested review from stdrc, TennyZhuang and BugenZhao May 14, 2023 16:50

xxchan requested a review from st1page May 14, 2023 16:57

BugenZhao reviewed May 15, 2023

View reviewed changes

src/stream/src/executor/over_window/eowc.rs Outdated Show resolved Hide resolved

xxchan requested a review from wangrunji0408 May 15, 2023 13:17

xxchan added 2 commits May 15, 2023 17:01

Merge remote-tracking branch 'origin/main' into xxchan/snapshot

1fe29e6

project set

74ee568

xxchan force-pushed the xxchan/snapshot branch from 4b304ef to 74ee568 Compare May 15, 2023 15:21

xxchan added 3 commits May 16, 2023 15:44

get rid of async_closure

f5fb028

I am yaml master!

95223d0

fix

fd5b33b

soundOfDestiny approved these changes May 17, 2023

View reviewed changes

kwannoel reviewed May 17, 2023

View reviewed changes

.../snapshots/risingwave_stream__executor__over_window__eowc__tests__over_window_aggregate.snap Outdated Show resolved Hide resolved

kwannoel approved these changes May 17, 2023

View reviewed changes

xxchan marked this pull request as ready for review May 17, 2023 08:19

add description & omit expression

5170b74

risingwavelabs deleted a comment from github-actions bot May 17, 2023

xxchan added 9 commits May 18, 2023 15:52

all in expect-test

d6b2d83

Merge remote-tracking branch 'origin/main' into xxchan/snapshot

a4ad151

unnecessary async

01d7514

remove snaps

d965739

Discard changes to src/stream/src/executor/top_n/top_n_plain.rs

db83df9

Delete insta.yaml

d096b56

Discard changes to src/stream/src/common/table/state_table.rs

aa2f3bc

minor

4ca30fb

remove note, as function comment contains notes

384d67f

xxchan mentioned this pull request May 18, 2023

test: split input/output for planner test #9902

Merged

1 task

stdrc reviewed May 19, 2023

View reviewed changes

xxchan added 3 commits May 19, 2023 09:27

rename it -> integration_tests

1124bc8

expect-test = "1"

5762414

Merge branch 'main' into xxchan/snapshot

9c6cd87

xxchan enabled auto-merge May 19, 2023 07:29

xxchan added this pull request to the merge queue May 19, 2023

Merged via the queue into main with commit 09d1849 May 19, 2023

xxchan deleted the xxchan/snapshot branch May 19, 2023 07:56

xxchan changed the title ~~test: snapshot testing for unary stream executors~~ test: snapshot testing for stream executors May 19, 2023

xxchan mentioned this pull request May 19, 2023

test facility: find a more ergonomic way to build executors #3847

Closed

stdrc mentioned this pull request Jun 1, 2023

refactor(test): move hash agg executor tests to integration_tests #10113

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: snapshot testing for stream executors #9787

test: snapshot testing for stream executors #9787

xxchan commented May 14, 2023 •

edited

Loading

xxchan commented May 14, 2023 •

edited

Loading

st1page commented May 14, 2023 •

edited

Loading

stdrc commented May 15, 2023

BugenZhao left a comment

xxchan commented May 15, 2023 •

edited

Loading

wangrunji0408 commented May 16, 2023

stdrc commented May 16, 2023 •

edited

Loading

xxchan commented May 16, 2023

soundOfDestiny left a comment

kwannoel left a comment

TennyZhuang commented May 17, 2023

codecov bot commented May 17, 2023 •

edited

Loading

stdrc commented May 17, 2023

BugenZhao commented May 18, 2023

xxchan commented May 18, 2023 •

edited

Loading

xxchan commented May 18, 2023 •

edited

Loading

stdrc left a comment

stdrc May 19, 2023

xxchan May 19, 2023

xxchan commented May 19, 2023

stdrc commented May 19, 2023

xxchan commented May 19, 2023

test: snapshot testing for stream executors #9787

test: snapshot testing for stream executors #9787

Conversation

xxchan commented May 14, 2023 • edited Loading

What's changed and what's your intention?

Checklist For Contributors

Checklist For Reviewers

Documentation

Types of user-facing changes

Release note

xxchan commented May 14, 2023 • edited Loading

st1page commented May 14, 2023 • edited Loading

stdrc commented May 15, 2023

BugenZhao left a comment

Choose a reason for hiding this comment

xxchan commented May 15, 2023 • edited Loading

wangrunji0408 commented May 16, 2023

stdrc commented May 16, 2023 • edited Loading

xxchan commented May 16, 2023

soundOfDestiny left a comment

Choose a reason for hiding this comment

kwannoel left a comment

Choose a reason for hiding this comment

TennyZhuang commented May 17, 2023

codecov bot commented May 17, 2023 • edited Loading

Codecov Report

stdrc commented May 17, 2023

BugenZhao commented May 18, 2023

xxchan commented May 18, 2023 • edited Loading

xxchan commented May 18, 2023 • edited Loading

stdrc left a comment

Choose a reason for hiding this comment

stdrc May 19, 2023

Choose a reason for hiding this comment

xxchan May 19, 2023

Choose a reason for hiding this comment

xxchan commented May 19, 2023

stdrc commented May 19, 2023

xxchan commented May 19, 2023

xxchan commented May 14, 2023 •

edited

Loading

xxchan commented May 14, 2023 •

edited

Loading

st1page commented May 14, 2023 •

edited

Loading

xxchan commented May 15, 2023 •

edited

Loading

stdrc commented May 16, 2023 •

edited

Loading

codecov bot commented May 17, 2023 •

edited

Loading

xxchan commented May 18, 2023 •

edited

Loading

xxchan commented May 18, 2023 •

edited

Loading