Repository: etcd-io/etcd
File: tests/robustness/validate/patch_history_test.go (Go)

Code before:
				h.AppendTxn(nil, []clientv3.Op{clientv3.OpDelete("key")}, []clientv3.Op{clientv3.OpPut("key", "value")}, 100, infinite, nil, errors.New("failed"))
			},
			expectedRemainingOperations: []porcupine.Operation{
				{Return: infinite + 100, Output: model.MaybeEtcdResponse{Error: "failed"}},
			},
		},
		{
			name: "failed txn empty/put is dropped",
			historyFunc: func(h *model.AppendableHistory) {
				h.AppendTxn(nil, []clientv3.Op{}, []clientv3.Op{clientv3.OpPut("key", "value")}, 100, infinite, nil, errors.New("failed"))
			},
			expectedRemainingOperations: []porcupine.Operation{},
		},
		{
			name: "failed txn empty/put remains if there is a matching event",
			historyFunc: func(h *model.AppendableHistory) {
				h.AppendTxn(nil, []clientv3.Op{clientv3.OpPut("key", "value")}, []clientv3.Op{}, 100, infinite, nil, errors.New("failed"))
			},
			persistedRequest: []model.EtcdRequest{
				putRequest("key", "value"),
			},
			expectedRemainingOperations: []porcupine.Operation{
				{Return: infinite + 99, Output: model.MaybeEtcdResponse{Persisted: true}},
			},
		},
		{
			name: "retried txn remains if there is matching persisted event, other failed txn time rebased accordingly",
			historyFunc: func(h *model.AppendableHistory) {
				h.AppendTxn([]clientv3.Cmp{clientv3.Compare(clientv3.ModRevision("key1"), "=", 0)}, []clientv3.Op{clientv3.OpPut("key1", "value1")}, []clientv3.Op{}, 100, infinite, nil, errors.New("rpc error: code = Unavailable desc = error reading from server: EOF"))
				h.AppendTxn(nil, []clientv3.Op{clientv3.OpPut("key2", "value2")}, []clientv3.Op{clientv3.OpDelete("key2")}, 200, infinite, nil, errors.New("failed"))
				h.AppendTxn([]clientv3.Cmp{clientv3.Compare(clientv3.ModRevision("key1"), "=", 0)}, []clientv3.Op{clientv3.OpPut("key1", "value1")}, []clientv3.Op{}, 400, 500, &clientv3.TxnResponse{Succeeded: false}, nil)
			},
			persistedRequest: []model.EtcdRequest{
				putRequest("key1", "value1"),
				putRequest("key2", "value2"),
				putRequest("key1", "value1"),
			},
			watchOperations: watchResponse(150, putEvent("key1", "value1", 1)),
			expectedRemainingOperations: []porcupine.Operation{
				{Return: infinite + 500, Output: model.MaybeEtcdResponse{Error: "rpc error: code = Unavailable desc = error reading from server: EOF"}},
				{Return: infinite + 498, Output: model.MaybeEtcdResponse{Error: "failed"}},
				{Return: 500, Output: model.MaybeEtcdResponse{EtcdResponse: model.EtcdResponse{Txn: &model.TxnResponse{Failure: true}}}},
			},
		},
		{
			name: "failed txn empty/delete remains",
			historyFunc: func(h *model.AppendableHistory) {
				h.AppendTxn(nil, []clientv3.Op{}, []clientv3.Op{clientv3.OpDelete("key")}, 100, infinite, nil, errors.New("failed"))
			},
			expectedRemainingOperations: []porcupine.Operation{
				{Return: infinite + 100, Output: model.MaybeEtcdResponse{Error: "failed"}},

Review comment (bug):
This test caches the error, however I think it could be simplified to make the case clearer, for example watch event is not needed here. What do you think about adding the following tests:

```
		{
			name: "failed put remains if there is a matching persisted request, uniqueness of this operation and following operation allows patching based on following operation",
			historyFunc: func(h *model.AppendableHistory) {
				h.AppendPut("key1", "value1", 300, infinite, nil, errors.New("failed"))
				h.AppendPut("key2", "value2", 500, 600, &clientv3.PutResponse{}, nil)
			},
			persistedRequest: []model.EtcdRequest{
				putRequest("key1", "value1"),
				putRequest("key2", "value2"),
			},
			expectedRemainingOperations: []porcupine.Operation{
				{Return: 599, Output: model.MaybeEtcdResponse{Persisted: true}},
				{Return: 600, Output: putResponse(model.EtcdOperationResult{})},
			},
		},
		{
			name: "failed put remains if there is a matching persisted request, lack of uniqueness of this operation prevents patching based on following operation",
			historyFunc: func(h *model.AppendableHistory) {
				h.AppendPut("key1", "value1", 100, 200, &clientv3.PutResponse{}, nil)
				h.AppendPut("key1", "value1", 300, infinite, nil, errors.New("failed"))
				h.AppendPut("key2", "value2", 500, 600, &clientv3.PutResponse{}, nil)
			},
			persistedRequest: []model.EtcdRequest{
				putRequest("key1", "value1"),
				putRequest("key1", "value1"),
				putRequest("key2", "value2"),
			},
			expectedRemainingOperations: []porcupine.Operation{
				{Return: 200, Output: putResponse(model.EtcdOperationResult{})},
				{Return: infinite + 600, Output: model.MaybeEtcdResponse{Error: "failed"}},
				{Return: 600, Output: putResponse(model.EtcdOperationResult{})},
			},
		},
		{
			name: "failed put remains if there is a matching persisted request, lack of uniqueness of following operation prevents patching based on following operation",
			historyFunc: func(h *model.AppendableHistory) {
				h.AppendPut("key2", "value2", 100, 200, &clientv3.PutResponse{}, nil)
				h.AppendPut("key1", "value1", 300, infinite, nil, errors.New("failed"))
				h.AppendPut("key2", "value2", 500, 600, &clientv3.PutResponse{}, nil)
			},
			persistedRequest: []model.EtcdRequest{
				putRequest("key2", "value2"),
				putRequest("key1", "value1"),
				putRequest("key2", "value2"),
			},
			expectedRemainingOperations: []porcupine.Operation{
				{Return: 200, Output: putResponse(model.EtcdOperationResult{})},
				// TODO: We can infer that failed operation finished before last operation matching following.
				{Return: infinite + 598, Output: model.MaybeEtcdResponse{Persisted: true}},
				{Return: 600, Output: putResponse(model.EtcdOperationResult{})},
			},
		},
```

First one shows that return time can be patched based on following operations. Second shows that it doesn't work if the operation it self is not unique, and your case that shows that patching should not work if the following operation is not unique. 

Writing down those tests also showed me a case that it still would make sense to patch if the following operation is not unique. We could technically use max return time from following operations.

Code after:
				h.AppendTxn(nil, []clientv3.Op{clientv3.OpDelete("key")}, []clientv3.Op{clientv3.OpPut("key", "value")}, 100, infinite, nil, errors.New("failed"))
			},
			expectedRemainingOperations: []porcupine.Operation{
				{Return: infinite + 100, Output: model.MaybeEtcdResponse{Error: "failed"}},
			},
		},
		{
			name: "failed txn empty/put is dropped",
			historyFunc: func(h *model.AppendableHistory) {
				h.AppendTxn(nil, []clientv3.Op{}, []clientv3.Op{clientv3.OpPut("key", "value")}, 100, infinite, nil, errors.New("failed"))
			},
			expectedRemainingOperations: []porcupine.Operation{},
		},
		{
			name: "failed txn empty/put remains if there is a matching event",
			historyFunc: func(h *model.AppendableHistory) {
				h.AppendTxn(nil, []clientv3.Op{clientv3.OpPut("key", "value")}, []clientv3.Op{}, 100, infinite, nil, errors.New("failed"))
			},
			persistedRequest: []model.EtcdRequest{
				putRequest("key", "value"),
			},
			expectedRemainingOperations: []porcupine.Operation{
				{Return: infinite + 99, Output: model.MaybeEtcdResponse{Persisted: true}},
			},
		},
		{
			name: "failed put remains if there is a matching persisted request, uniqueness of this operation and following operation allows patching based on following operation",
			historyFunc: func(h *model.AppendableHistory) {
				h.AppendPut("key1", "value1", 300, infinite, nil, errors.New("failed"))
				h.AppendPut("key2", "value2", 500, 600, &clientv3.PutResponse{}, nil)
			},
			persistedRequest: []model.EtcdRequest{
				putRequest("key1", "value1"),
				putRequest("key2", "value2"),
			},
			expectedRemainingOperations: []porcupine.Operation{
				{Return: 599, Output: model.MaybeEtcdResponse{Persisted: true}},
				{Return: 600, Output: putResponse(model.EtcdOperationResult{})},
			},
		},
		{
			name: "failed put remains if there is a matching persisted request, lack of uniqueness of this operation prevents patching based on following operation",
			historyFunc: func(h *model.AppendableHistory) {
				h.AppendPut("key1", "value1", 100, 200, &clientv3.PutResponse{}, nil)
				h.AppendPut("key1", "value1", 300, infinite, nil, errors.New("failed"))
				h.AppendPut("key2", "value2", 500, 600, &clientv3.PutResponse{}, nil)
			},
			persistedRequest: []model.EtcdRequest{
				putRequest("key1", "value1"),
				putRequest("key1", "value1"),
				putRequest("key2", "value2"),