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

QCHECK_LONG_FACTOR environment variable #220

Merged
merged 5 commits into from
May 5, 2022
Merged

QCHECK_LONG_FACTOR environment variable #220

merged 5 commits into from
May 5, 2022

Conversation

vch9
Copy link
Contributor

@vch9 vch9 commented Jan 7, 2022

In #142 we introduced a QCHECK_COUNT environment variable. It is very useful to debug flaky tests by running a dedicated pipeline with a lot higher QCHECK_COUNT. We decided that the ~count in a Test.make overrides the environment variable (which I think is fine), but, there is no easy way to conditionally increase these given counts.

I propose another environment variable QCHECK_COUNT_MULTIPLIER to increase the ~count value.

@c-cube
Copy link
Owner

c-cube commented Jan 7, 2022

just a remark, there is already the notion of long tests, which is a multiplier. Is this different? (the env var is a good idea anyway, mind you).

@vch9
Copy link
Contributor Author

vch9 commented Jan 7, 2022

just a remark, there is already the notion of long tests, which is a multiplier. Is this different? (the env var is a good idea anyway, mind you).

I was not aware of long tests, what I proposed is not different. I'll change to an env variable QCHECK_LONG_FACTOR yes

@vch9 vch9 changed the title QCHECK_COUNT_MULTIPLIER environment variable QCHECK_LONG_FACTOR environment variable Jan 10, 2022
src/core/QCheck2.ml Outdated Show resolved Hide resolved
@vch9
Copy link
Contributor Author

vch9 commented May 5, 2022

Hi @c-cube and/or @jmid :).
I've rebased this branch and add additional tests. We'd be interesting on using this for the Tezos CI, do you think you could have a look?

Copy link
Collaborator

@jmid jmid left a comment

Choose a reason for hiding this comment

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

Overall this is a very reasonable request of being able to configure long_factor through an environment variable. Code looks good and clean too. My heartfelt thanks (really!) for including tests of the new functionality! 🙏

If I'm not mistaken, this only adds the functionality for QCheck2 - which leaves us in a bit of a limbo. Could I persuade you to copy-paste the functionality so that it will also work for QCheck tests? (the unit-tests for QCheck should largely also just be a copy-paste job)

Finally, you are also welcome to add a CHANGELOG entry for the new functionality.

src/core/QCheck2.ml Show resolved Hide resolved
@vch9
Copy link
Contributor Author

vch9 commented May 5, 2022

Overall this is a very reasonable request of being able to configure long_factor through an environment variable. Code looks good and clean too. My heartfelt thanks (really!) for including tests of the new functionality! pray

If I'm not mistaken, this only adds the functionality for QCheck2 - which leaves us in a bit of a limbo. Could I persuade you to copy-paste the functionality so that it will also work for QCheck tests? (the unit-tests for QCheck should largely also just be a copy-paste job)

Finally, you are also welcome to add a CHANGELOG entry for the new functionality.

You're absolutely right.

  • I made the changes needed for QCheck in 8f3d032
  • Updated the changelog in 5b30869, I also agree that it is a good thing to update the changelog every time we add a feature

@jmid
Copy link
Collaborator

jmid commented May 5, 2022

Great, thank!
Extra credit for also adding a corresponding TestCount module for the QCheck(1) unit tests, yay! 👏 😃

@jmid jmid merged commit 978eea6 into c-cube:master May 5, 2022
@vbgl vbgl mentioned this pull request Jul 13, 2022
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

Successfully merging this pull request may close these issues.

3 participants