test: add error check after cacheOp.Exec

[piepor]

Previously, any error sent by cacheOp.Exec(ctx, errCh) to errCh was ignored; this change explicitly checks the error and handles it gracefully.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.63%. Comparing base (01a9618) to head (cf04692).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4055      +/-   ##
==========================================
- Coverage   70.68%   70.63%   -0.05%     
==========================================
  Files         460      461       +1     
  Lines       53796    53899     +103     
==========================================
+ Hits        38021    38068      +47     
- Misses      12822    12862      +40     
- Partials     2953     2969      +16     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ngjaying]

I don’t think this fixed delay is useful here. TestCacheRun already validates correctness through the processed metrics and expected outputs. Since cacheOp.Exec starts async work,
checking errCh once after 100ms only samples one moment in time and does not catch errors emitted later while the test is actually exercising the operator.

It also makes the test slower without adding a deterministic readiness signal. What specific failure is this intended to fix? If we want to validate async operator errors, the waits/
receives in the test should select on errCh while waiting for processing/output. Otherwise I’d prefer to remove this block.

[piepor]

The problem I was trying to fix is a segmentation fault panic: runtime error: invalid memory address or nil pointer dereference during TestCacheRun: if cacheOp.Exec fails due to en error in InitStore (failure mode in my case) s.prepareExec is skipped and the StatManager is not created.

func (s *CacheOp) Exec(ctx api.StreamContext, errCh chan<- error) {
	if len(s.outputs) > 1 {
		infra.DrainError(ctx, fmt.Errorf("cache op should have only 1 output but got %+v", s.outputs), errCh)
	}
	s.cache.SetupMeta(ctx)
	if err := s.cache.InitStore(ctx); err != nil {
		infra.DrainError(ctx, fmt.Errorf("cache op init store error:%v", err), errCh)
		return
	}
	s.prepareExec(ctx, errCh, "op")
	go func() {
        ...
  }()
}

Consequently, cacheOp.statManager.GetMetrics() does not exists: hence the nil pointer issue when it gets called during the tests in TestCacheRun:

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			var r any
			for index < tt.sendUntil {
				cacheOp.input <- tuples[index]
				index++
			}
			timex.Add(100 * time.Millisecond)
			for { // wait until all processed
				processed := cacheOp.statManager.GetMetrics()[2]
				if processed == int64(tt.sendUntil) {
					break
				}
				time.Sleep(10 * time.Millisecond)
			}
      ... 
      }

I agree the actual test has an unnecessary delay: perhaps we could check for the two synchronous errors of cacheOp.Exec (caused by len(outputs) > 1 and err := s.cache.InitStore) without delay and then requiring that cacheOp.statManager does exits before running tests in the loop?