Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
epoching: more test infra and some fuzz tests on keeper functionalities #60
epoching: more test infra and some fuzz tests on keeper functionalities #60
Changes from 78 commits
951dcf3
af5ff8d
e12dd3f
d6333c1
6cd242a
d997725
82eb108
d6d5996
a91c059
6bc1fb3
729da0b
3d5f5a1
a6aa599
34caaa5
97c36cd
8818a67
1b8de2f
3273562
bf09eb8
39b7d61
a06bbc1
602ffee
a766236
8a6a88c
94e91e6
fe44d45
be19c4f
12cfbf6
8b014f3
6e7872a
54c3292
dc8b09a
7c8fe31
b337667
6bad046
5d2cd5d
ebdc449
7ec30d4
8f23255
26b04af
237121f
8fb65d7
805c431
a2405f2
05ed1ae
2ec73fb
e50688c
eb2b31d
f8ca14c
64a3cc1
d3dcfb8
bb7ca42
0439e57
9d0b65d
69164ba
29c3c2d
1808729
4a31892
af3e327
9750d21
509c09e
abcdcaa
522d7f4
c8c60b0
4ae0a62
0565c96
2cea378
82dd3ae
fbbcd4c
a82ef68
b739661
fd1383f
faaada2
6d05839
f3a1f28
efb1dde
e7d7f45
3278402
3fc6cab
a6cd3d6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why bother with these? If something fails, I thought the application will save it into a place where it can act as a regression test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed the case when we run individual fuzz tests. However, CI will only run the test cases added by
f.Add
without trying other random values generated from the corpus. Will having more corpus be beneficial for CI? If not then I will keep only one of them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so you want it to run at least a few times. We discussed with @vitsalis that we could add
-fuzztime
to the CI to run it for a limited amount of iterations.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you guys think about doing something like
f.Add(time.Now().UnixMilli())
so the tests run as unit tests are not always the same, and you aren't forced to include this arbitrary looking boilerplate all the time. It would ensure that it runs them at least once, and if they fail then hopefully it prints the troublesome seed as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The counter argument was that it makes builds non-deterministic. It can be annoying if random tests fail sometimes, not other times, but over a long period of time it would catch more bugs than relying on the same seeds. We could even do something like
addRandomSeeds(f, 100)
to add exactly 100 random seeds, which is what other frameworks are doing (ie. they achieve 100 passes by default for each test, rather than run forever like Go).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ScalaCheck has CLI flags for the numbers, so maybe we could also support some env vars to adjust defaults. Although I never needed that, I was happy with 100 unless it took really long to generate the data and I had to lower it on a per-test or per-suite basis, like here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you aware of any projects using such strategy? I feel that this requires a very powerful VM to run the CI. My CPU was super hot when executing fuzz tests and sometimes a fuzz test takes >10s to generate the initial corpus.
This is a good idea. Basically we limit the number of tries (rather than the running time) for fuzz tests. I will look into this approach and find a way to try it in the subsequent PRs. Hopefully the CI will be happy to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the QuickCheck family runs 100 tests by default and it's up to you to tweak the numbers. I also want tests to finish in reasonable time, max 10 seconds, so if I see that they take longer then I have to reign in how much data I generate.
Maybe I don't need 1000 blocks with up to 100 transactions in each just to test something simple, and maybe I let my tree generation do too much branching and my tree of 10 depth is exponentially wide. Generating random bytes can also get out of hand, like opaque payloads, we don't need them to be hundreds of kilobytes long.
So it's a good indication of where time is not spent well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And you are right I'd rather know that a particular test passed 100 runs than that some VM spent overall 1 minute testing all fuzz tests and may or may not have covered them equally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be worth creating a constructor method like
NewQueuedMessage(txid, msg)
and let it calculate the hash itself, so we're not able to generate invalid ones. Just an idea.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay but why would it change? There are no transactions, and even if there were the test doesn't say how long an epoch is.
Shouldn't the test generate random registrations and check that the validator set doesn't change during an epoch, but it does at the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea! However this requires mocking
MsgCreateValidator
or evenMsgWrappedCreateValidator
that includes BLS stuff. I have added a TODO on this and will do it in the subsequent PRs.