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.

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

tests: Support multiple keys in linearizability tests #14924

Merged
merged 2 commits into from
Dec 29, 2022

Conversation

serathius
Copy link
Member

@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2022

Codecov Report

Merging #14924 (d4c8611) into main (a4c1b3a) will decrease coverage by 0.37%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #14924      +/-   ##
==========================================
- Coverage   74.92%   74.55%   -0.38%     
==========================================
  Files         415      415              
  Lines       34276    34276              
==========================================
- Hits        25682    25554     -128     
- Misses       6970     7072     +102     
- Partials     1624     1650      +26     
Flag Coverage Δ
all 74.55% <ø> (-0.38%) ⬇️

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

Impacted Files Coverage Δ
server/etcdserver/api/etcdhttp/types/errors.go 60.00% <0.00%> (-13.34%) ⬇️
client/pkg/v3/testutil/leak.go 62.83% <0.00%> (-7.08%) ⬇️
client/v3/experimental/recipes/queue.go 58.62% <0.00%> (-6.90%) ⬇️
server/storage/mvcc/watchable_store.go 85.14% <0.00%> (-6.53%) ⬇️
server/lease/lease.go 94.87% <0.00%> (-5.13%) ⬇️
client/v3/leasing/cache.go 87.22% <0.00%> (-4.45%) ⬇️
client/v3/concurrency/session.go 85.10% <0.00%> (-4.26%) ⬇️
server/etcdserver/api/v3rpc/watch.go 84.12% <0.00%> (-4.13%) ⬇️
server/proxy/grpcproxy/watch.go 92.48% <0.00%> (-4.05%) ⬇️
server/storage/mvcc/watcher.go 96.29% <0.00%> (-3.71%) ⬇️
... and 20 more

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

tests/linearizability/model.go Outdated Show resolved Hide resolved
tests/linearizability/model.go Outdated Show resolved Hide resolved
tests/linearizability/model.go Outdated Show resolved Hide resolved
tests/linearizability/model.go Show resolved Hide resolved
tests/linearizability/model.go Outdated Show resolved Hide resolved
@serathius
Copy link
Member Author

Resolved all comments that do not make sense. Please describe what you would like to change and not send 20 suggestions that will not work.

Suggestions make sense only if:

i++
}
}
state.PossibleValues = state.PossibleValues[:i]
states = states[:i]
Copy link
Contributor

Choose a reason for hiding this comment

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

clever idea with the state reduction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, but looks like it's not very readable. Rewrote it to make the code cleaner.

Copy link
Contributor

@tjungblu tjungblu left a comment

Choose a reason for hiding this comment

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

lgtm (non-binding)

@serathius serathius force-pushed the linearizability-multiple-keys branch 2 times, most recently from f7f440a to 2cd4fba Compare December 12, 2022 16:09
@ahrtr
Copy link
Member

ahrtr commented Dec 12, 2022

Overall looks good to me. The only concern is the documentation.

The model is the most important part of the linearizablity test, so please add detailed document/comment to describe the overall thought and make sure the document/comment is always up-to-date, especially you are changing the code too fast and frequently. I should raise this comment in your last PR in which you totally changed the implementation. Otherwise others may spend lots of time to understand your thoughts.

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

@serathius
Copy link
Member Author

Decided to separate PR that refactors the model first. Let's discuss documenting model there. #15045

@serathius serathius force-pushed the linearizability-multiple-keys branch 3 times, most recently from 55fd9b1 to 7b891f9 Compare December 24, 2022 20:17
@ahrtr
Copy link
Member

ahrtr commented Dec 26, 2022

Please add self-contained comment in model.go to describe the overall thought. And also add comment something like below for stepState,

`stepState` generates all the possible expected states and expected response based on all the current states. 
The caller can compare the expected response with the actual response, and take action accordingly.

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
@serathius serathius force-pushed the linearizability-multiple-keys branch 2 times, most recently from 8e181b2 to ab093f5 Compare December 28, 2022 20:39
@serathius
Copy link
Member Author

@ahrtr Done, please take a look.

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
Co-authored-by: Benjamin Wang <wachao@vmware.com>
@serathius serathius merged commit 18a5dc4 into etcd-io:main Dec 29, 2022
@serathius serathius deleted the linearizability-multiple-keys branch June 15, 2023 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants