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

QSM DB tests don't check that all tags are covered. #353

Closed
1 task done
jonathanknowles opened this issue Jun 4, 2019 · 5 comments
Closed
1 task done

QSM DB tests don't check that all tags are covered. #353

jonathanknowles opened this issue Jun 4, 2019 · 5 comments
Assignees

Comments

@jonathanknowles
Copy link
Member

jonathanknowles commented Jun 4, 2019

Release Operating System Cause
next Windows & OSX & Linux) Code

Context

Module Cardano.Wallet.DB.StateMachine defines set of Tag values to represent various test scenarios that are considered important or interesting. It also supplies a tag function to match Tag values to arbitrary sequences of commands.

However, the test suite doesn't actually check that all possible Tag values are covered during execution. This creates a risk that we might actually not be covering all the cases we'd like. (For example: see PR #349, which fixes a bug where CreateThreeWallets was never generated.)

Resolution Plan

  • Adjust prop_sequential so that it fails if one or more tags are not covered.

PR

Number Base
#367 develop

QA

@jonathanknowles
Copy link
Member Author

Fix available: #367

@KtorZ KtorZ added this to the Bugs Sprint 21-22 milestone Jun 5, 2019
@rvl
Copy link
Contributor

rvl commented Jun 5, 2019

If this issue is called a BUG, then to avoid BUGs, I would have to keep my PRs open until everything has been implemented fully and therefore free of BUGs.

@KtorZ
Copy link
Member

KtorZ commented Jun 5, 2019

@rvl Don't feel attacked with the word "bug", maybe it's too strong of a word. In the workflow, we simply open bugs for oversights or, things we caught after a task was completed or merged. This allows us to keep track of the work more thoroughly than having PR staying opened for too long.

@piotr-iohk
Copy link
Contributor

lgtm 👍

@jonathanknowles
Copy link
Member Author

jonathanknowles commented Jun 6, 2019

@KtorZ @rvl

Perhaps I can explain my reasoning for adding the "bug" tag here:

  1. With a traditional test suite, we expect to see failure if one or more test cases don't pass.
  2. If a traditional test suite runs to completion reporting success ✅, but silently fails to run one or more test cases, then we'd probably consider it a bug, as it violates our expectations of what a test suite is supposed to do.
  3. When we go from traditional testing to state machine-based testing, hand-written test cases (to test scenarios we care about) are replaced with tags and a tag matching function (again, to check that we're testing scenarios that we care about).
  4. If a tag doesn't match during a given run, then it could be considered equivalent to a test case in a traditional test suite not being run.
  5. If a state machine test runs to completion without matching a given tag and yet still reports success, this is equivalent to a traditional test suite running to completion without running all the tests but still reporting success.

So that is why I added the "bug" tag in this case. (Feel free to point out holes in my reasoning!)

However I can understand @rvl's argument here, that we have to stop somewhere when making a PR, or else PRs will always be in an incomplete state. The original QSM DB PR #259 was already a huge amount of work. (We can always submit further PRs to make something better.)

Therefore, I could understand if people wanted to label something like this as an "improvement".

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants