Prevent regressions #1243
Prevent regressions #1243
Conversation
sashashura
force-pushed
the
regr
branch
4 times, most recently
from
11289d2 to
359beb2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1243 +/- ##
==========================================
- Coverage 82.72% 82.71% -0.01%
==========================================
Files 159 159
Lines 20404 20404
Branches 7711 7711
==========================================
- Hits 16879 16878 -1
- Misses 2903 2914 +11
+ Partials 622 612 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
sashashura
force-pushed
the
regr
branch
6 times, most recently
from
451cb4f to
5e8b8ad
Compare
|
This pr uncomments memory sanitizer regressions tests and adds a quick fuzzing tests. I see you had concerns in the past that that CIFuzz may give alerts not related to the pr and make for people harder to contribute. However it is written in the documentation:
So if it is something that can be reproduced with the old build it won't fail. |
What does an "older build" mean? I'd expect that CIFuzz will try to reproduce the crash on the branch the PR will be merged to (in our case - it's |
|
I guess so. Is there anything wrong about it? It is quite effective
strategy - even if the crash wasn't known before, just rerun the single
reproducer case. If it reliably crashes with the target branch, then it
wasn't introduced with the pr and there is no need to fail the check.
…On Wed, Nov 22, 2023, 02:04 seladb ***@***.***> wrote:
This pr uncomments memory sanitizer regressions tests and adds a quick
fuzzing tests. I see you had concerns in the past that
<#761> that CIFuzz may give
alerts not related to the pr and make for people harder to contribute.
However it is written in the documentation: If CIFuzz doesn’t find a crash
during the allotted time, the CI test passes (green check). If CIFuzz finds
a crash, it reports the crash only if both of following are true:
* The crash is reproducible (on the PR/commit build).
* The crash does not occur on older OSS-Fuzz builds. (If the crash does occur on older builds, then it was not introduced by the PR/commit being tested.)
So if it is something that can be reproduced with the old build it won't
fail.
What does an "older build" mean? I'd expect that CIFuzz will try to
reproduce the crash on the branch the PR will be merged to (in our case -
it's dev most of the time). Is that the case?
—
Reply to this email directly, view it on GitHub
<#1243 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AWINCMSF6PAKSRSGB7GJVELYFVFSXAVCNFSM6AAAAAA7RJAFS6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRRHEZTAMRQGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I'd like to make sure that if the CIFuzz run fails it's only because of something related to the changes in the PR. The only way to verify is to run the same scenario on the branch the PR will be merged to and make sure it doesn't crash. Is there any way to make sure? What I'd like to avoid is a scenario where CIFuzz discovered a crash in a certain PR that is also reproducible in |
|
@selab they are lot of projects using CiFuzz and not small ones. I would say let's try it and if this kind of issue happens we can always disable it. My understanding is that in case of new fuzzing discovery they redo the test with the previous commit just to be sure it's not an improvement of the fuzzing engine. |
| - name: Compile fuzzer | ||
| run: | | ||
| export FUZZING_LANGUAGE=c | ||
| export ARCHITECTURE=x86_64 |
There was a problem hiding this comment.
Nitpick but I'm not fan of using environment variable to pass value to a script.
I prefer to properly pass it as parameters, so we can properly check the input, have default and explain what we want.
But not a big deal as it's only for CI and not related to this MR
It seems this is how it already works. |
No description provided.