fix: benchmark and propagate the status to not to swallow the failure

[jcchavezs]

At some point the benchmaks got broken (can't access action logs older than 3 months) but could have been broken for a while.

BenchmarkCSSDecode/test\a\b\f\n\r\t\v\?\'\"\\x00\12\123\1234\12345\123456\ff01\ff5e\_\\x00__string-2         	 5382614	       222.9 ns/op
BenchmarkEscapeSeqDecode/#00-2                                                                               	229572150	         5.227 ns/op
BenchmarkEscapeSeqDecode/hello_world-2                                                                       	217793412	         5.508 ns/op
panic: runtime error: index out of range [44] with length 43

goroutine 460 [running]:
github.com/corazawaf/coraza/v3/internal/transformations.doEscapeSeqDecode({0x5e2241, 0x2b}, 0x0)
	/home/runner/work/coraza/coraza/internal/transformations/escape_seq_decode.go:101 +0x3a7
github.com/corazawaf/coraza/v3/internal/transformations.escapeSeqDecode({0x5e2241, 0x2b})
	/home/runner/work/coraza/coraza/internal/transformations/escape_seq_decode.go:17 +0x58
github.com/corazawaf/coraza/v3/internal/transformations.BenchmarkEscapeSeqDecode.func1(0xc00007c780)
	/home/runner/work/coraza/coraza/internal/transformations/escape_seq_decode_test.go:59 +0x65
testing.(*B).runN(0xc00007c780, 0x1)
	/opt/hostedtoolcache/go/1.20.4/x64/src/testing/benchmark.go:193 +0x[102](https://github.com/corazawaf/coraza/actions/runs/5210566284/jobs/9401796344?pr=808#step:5:103)
testing.(*B).run1.func1()
	/opt/hostedtoolcache/go/1.20.4/x64/src/testing/benchmark.go:233 +0x59
created by testing.(*B).run1
	/opt/hostedtoolcache/go/1.20.4/x64/src/testing/benchmark.go:226 +0x9c
exit status 2
FAIL	github.com/corazawaf/coraza/v3/internal/transformations	24.522s

See https://github.com/corazawaf/coraza/actions/runs/5210566284/jobs/9401796344?pr=808#step:5:97

cc @anuraaga @jptosso as they worked on this mainly

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02 ⚠️

Comparison is base (e1b119b) 81.58% compared to head (0a52de3) 81.57%.

❗ Current head 0a52de3 differs from pull request most recent head 9e9ed45. Consider uploading reports for the commit 9e9ed45 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #808      +/-   ##
==========================================
- Coverage   81.58%   81.57%   -0.02%     
==========================================
  Files         158      158              
  Lines        8966     8959       -7     
==========================================
- Hits         7315     7308       -7     
  Misses       1406     1406              
  Partials      245      245              

Flag	Coverage Δ
default	76.56% <100.00%> (+0.30%)	⬆️
examples	25.66% <0.00%> (+0.02%)	⬆️
ftw	47.11% <100.00%> (-0.05%)	⬇️
ftw-multiphase	49.25% <100.00%> (-0.05%)	⬇️
tinygo	72.64% <100.00%> (+0.32%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
internal/transformations/escape_seq_decode.go	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[anuraaga]

Always had trouble understanding the escape_seq_decode logic so just refactored it completely, notably continue makes the logic flow easier to follow IMO. Not sure what part of it fixed the bug but looks like it did :P

[anuraaga]

@jcchavezs I think you can finish, we should remove the actual performance regression check. GitHub runners machine configuration / busyness varies wildly across runs, it is not possible to do a automated perf check on them without custom runners. So the best we can do is just compute the numbers somewhere as a reference point, that's what I've been doing in wasilibs

https://github.com/wasilibs/go-re2/blob/main/.github/workflows/bench.yaml

[jcchavezs]

Shopuld this be a benchmark or a test?

[anuraaga]

Not sure the question, but there should be a benchmark. It may also need to be a normal unit test.

[fzipi]

From the title of this PR I didn't expect the internal/transformations/escape_seq_decode.go to change in this PR. Can we create a new one, just for that change, and write a test if that is a fix?

[jcchavezs]

The change in the yaml file makes the Ci fail (see commit 95804f3) and then the fix in the code makes this green. I just changed the title, does that look better @fzipi ?

[fzipi]

I understand the pipe propagation for fixing the benchmark.

I don't understand why escape_seq_decode needed to change for a benchmark. Was this a bug in the code?

[jcchavezs]

Yes, benchmark was failing and since we were not propagating the status the CI was green.

…

On Thu, 15 Jun 2023, 16:49 Felipe Zipitría, ***@***.***> wrote: I understand the pipe propagation for fixing the benchmark. I don't understand why escape_seq_decode needed to change for a benchmark. Was this a bug in the code? — Reply to this email directly, view it on GitHub <#808 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXOYAUHXVKFSZKWBYTQ7I3XLMOGNANCNFSM6AAAAAAY7EWTDY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[fzipi]

Looks like we are not in sync.

[fzipi]

Do we need to add additional tests here

coraza/internal/transformations/escape_seq_decode_test.go

Line 8 in 4319daa

func TestEscapeSeqDecode(t *testing.T) {

?

Looks like we found a bug of some sort, and the failing benchmarks uncovered it. Do we need additional tests?

[jcchavezs]

@fzipi added the test.