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

test: add key assignment to model and driver #1573

Merged
merged 40 commits into from
Jan 29, 2024
Merged

Conversation

p-offtermatt
Copy link
Contributor

@p-offtermatt p-offtermatt commented Jan 19, 2024

Description

Closes: #1529

  • Adds key assignment to the Quint model
  • Adds key assignment to the MBT driver

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • Included the correct type prefix in the PR title
  • Targeted the correct branch (see PR Targeting)
  • Provided a link to the relevant issue or specification
  • Reviewed "Files changed" and left comments if necessary
  • Confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • Confirmed the correct type prefix in the PR title
  • Confirmed all author checklist items have been addressed
  • Confirmed that this PR does not change production code

@p-offtermatt p-offtermatt requested a review from a team as a code owner January 19, 2024 09:46
@github-actions github-actions bot added the C:Testing Assigned automatically by the PR labeler label Jan 19, 2024
@p-offtermatt p-offtermatt changed the title tests: add key assignment to model and driver test: add key assignment to model and driver Jan 19, 2024
tests/mbt/model/README.md Outdated Show resolved Hide resolved
tests/mbt/model/ccv.qnt Outdated Show resolved Hide resolved
Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

Great work @p-offtermatt. Partial review without ccv_model.qnt. See my comments below. In general, try to either comment large blocks of quint or split the blocks -- it's very hard for me (Quint beginner) to read it.

testutil/integration/validators.go Outdated Show resolved Hide resolved
tests/mbt/model/README.md Show resolved Hide resolved
tests/mbt/model/ccv.qnt Outdated Show resolved Hide resolved
tests/mbt/model/ccv.qnt Outdated Show resolved Hide resolved
tests/mbt/model/ccv.qnt Outdated Show resolved Hide resolved
tests/mbt/model/ccv.qnt Outdated Show resolved Hide resolved
tests/mbt/model/ccv.qnt Outdated Show resolved Hide resolved
tests/mbt/model/ccv_model.qnt Show resolved Hide resolved
tests/mbt/model/ccv.qnt Outdated Show resolved Hide resolved
@p-offtermatt p-offtermatt linked an issue Jan 25, 2024 that may be closed by this pull request
tests/mbt/model/ccv.qnt Outdated Show resolved Hide resolved
Copy link
Contributor

@insumity insumity left a comment

Choose a reason for hiding this comment

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

Great work!

I focused my review on the Quint code.
I concur with Mariu's comment in regards to introducing more comments in general.

Two last comments:

  1. I'm not sure if it would simplify the model in not considering 0 powers. If I understood correctly from our discussion, this stems from ABCI which might be too implementation specific. If it doesn't simplify, then it's fine to leave as is.
  2. Now addr and key seem to be used interchangeably in names. Would it make sense to stick with one for consistency?

@p-offtermatt
Copy link
Contributor Author

Great work!

I focused my review on the Quint code. I concur with Mariu's comment in regards to introducing more comments in general.

Two last comments:

1. I'm not sure if it would simplify the model in not considering `0` powers. If I understood correctly from our discussion, this stems from ABCI which might be too implementation specific. If it doesn't simplify, then it's fine to leave as is.

2. Now `addr` and `key` seem to be used interchangeably in names. Would it make sense to stick with one for consistency?
  1. Yes, I think this would simplify a lot. Let me do that - it will be some bigger changes, e.g. I can get rid of keyAssignmentReplacements afaict.
  2. I think both are sort-of needed: The feature is called key assignment, but in the model I think I don't want to talk about keys, but rather addresses, but maybe this is confusing. Let me think about this a bit, but feel free to chime in if you have an opinion @mpoke @sainoe

@github-actions github-actions bot added the C:Build Assigned automatically by the PR labeler label Jan 26, 2024
Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

Great work @p-offtermatt.

tests/mbt/model/ccv_model.qnt Outdated Show resolved Hide resolved
@sainoe sainoe self-requested a review January 29, 2024 07:35
@p-offtermatt p-offtermatt added this pull request to the merge queue Jan 29, 2024
Merged via the queue into main with commit d60b880 Jan 29, 2024
16 checks passed
@p-offtermatt p-offtermatt deleted the ph/mbt-keyassignment branch January 29, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Build Assigned automatically by the PR labeler C:Testing Assigned automatically by the PR labeler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MBT: Add key assignment Add key assignment logic to Quint model
4 participants