[
https://issues.apache.org/jira/browse/BEAM-6825?focusedWorklogId=224691&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-224691
]
ASF GitHub Bot logged work on BEAM-6825:
----------------------------------------
Author: ASF GitHub Bot
Created on: 09/Apr/19 00:03
Start Date: 09/Apr/19 00:03
Worklog Time Spent: 10m
Work Description: youngoli commented on pull request #8243: [BEAM-6825]
Improve Combine error messages.
URL: https://github.com/apache/beam/pull/8243#discussion_r273281696
##########
File path: sdks/go/pkg/beam/core/graph/fn_test.go
##########
@@ -0,0 +1,297 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package graph
+
+import (
+ "context"
+ "reflect"
+ "testing"
+)
+
+func TestNewCombineFn(t *testing.T) {
+ t.Run("valid", func(t *testing.T) {
+ tests := []struct {
+ cfn interface{}
+ }{
+ {cfn: func(int, int) int { return 0 }},
+ {cfn: func(string, string) string { return "" }},
+ {cfn: func(MyAccum, MyAccum) MyAccum { return MyAccum{}
}},
+ {cfn: func(MyAccum, MyAccum) (MyAccum, error) { return
MyAccum{}, nil }},
+ {cfn: func(context.Context, MyAccum, MyAccum) MyAccum {
return MyAccum{} }},
+ {cfn: func(context.Context, MyAccum, MyAccum) (MyAccum,
error) { return MyAccum{}, nil }},
+ {cfn: &GoodCombineFn{}},
+ {cfn: &GoodWErrorCombineFn{}},
+ {cfn: &GoodWContextCombineFn{}},
+ {cfn: &GoodCombineFnUnexportedExtraMethod{}},
+ }
+
+ for _, test := range tests {
+ t.Run(reflect.TypeOf(test.cfn).String(), func(t
*testing.T) {
+ if _, err := NewCombineFn(test.cfn); err != nil
{
+ t.Fatalf("NewCombineFn failed: %v", err)
+ }
+ })
+ }
+ })
+ t.Run("invalid", func(t *testing.T) {
+ tests := []struct {
+ cfn interface{}
+ }{
+ // Validate MergeAccumulator errors
+ {cfn: func() int { return 0 }},
+ {cfn: func(int, int) {}},
+ {cfn: func(int, int) string { return "" }},
+ {cfn: func(string, string) int { return 0 }},
+ {cfn: func(int, string) int { return 0 }},
+ {cfn: func(string, int) int { return 0 }},
+ {cfn: func(string, int) (int, error) { return 0, nil }},
+ {cfn: &BadCombineFnNoMergeAccumulators{}},
+ {cfn: &BadCombineFnNonBinaryMergeAccumulators{}},
+ // Validate accumulator type mismatches
+ {cfn: &BadCombineFnMisMatchedCreateAccumulator{}},
+ {cfn: &BadCombineFnMisMatchedAddInputIn{}},
+ {cfn: &BadCombineFnMisMatchedAddInputOut{}},
+ {cfn: &BadCombineFnMisMatchedAddInputBoth{}},
+ {cfn: &BadCombineFnMisMatchedExtractOutput{}},
+ // Validate signatures
+ {cfn: &BadCombineFnInvalidCreateAccumulator1{}},
+ {cfn: &BadCombineFnInvalidCreateAccumulator2{}},
+ {cfn: &BadCombineFnInvalidCreateAccumulator3{}},
+ {cfn: &BadCombineFnInvalidCreateAccumulator4{}},
+ {cfn: &BadCombineFnInvalidAddInput1{}},
+ {cfn: &BadCombineFnInvalidAddInput2{}},
+ {cfn: &BadCombineFnInvalidAddInput3{}},
+ {cfn: &BadCombineFnInvalidAddInput4{}},
+ {cfn: &BadCombineFnInvalidExtractOutput1{}},
+ {cfn: &BadCombineFnInvalidExtractOutput2{}},
+ {cfn: &BadCombineFnInvalidExtractOutput3{}},
+ {cfn: &BadCombineFnExtraExportedMethod{}},
+ }
+ for _, test := range tests {
+ t.Run(reflect.TypeOf(test.cfn).String(), func(t
*testing.T) {
+ if cfn, err := NewCombineFn(test.cfn); err !=
nil {
+ // DO NOT SUBMIT make into a Logf once
errors are clear enough.
+ // Note to Developer: To work on
improving the error messages, use t.Errorf instead!
+ t.Logf("NewCombineFn failed as
expected:\n%v", err)
+ } else {
+ t.Errorf("AsCombineFn(%v) = %v, want
failure", cfn.Name(), cfn)
+ }
+ })
+ }
+ })
+}
+
+// Do not copy. The following types are for testing signatures only.
+// They are not working examples.
+type MyAccum struct{}
+
+type GoodCombineFn struct{}
Review comment:
Nitpick, but all these structs blend together, making reading through this a
pain for anyone that edits this file in the future. A way to break up the flow
would be nice.
Thing is, the only way I can think of though is to add comments above each
struct declaration, but the comments wouldn't really add any actual info, and
it feels odd to suggest adding useless comments just to improve the formatting.
Are there better alternatives?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 224691)
Time Spent: 40m (was: 0.5h)
> Improve pipeline construction time error messages in Go SDK.
> ------------------------------------------------------------
>
> Key: BEAM-6825
> URL: https://issues.apache.org/jira/browse/BEAM-6825
> Project: Beam
> Issue Type: New Feature
> Components: sdk-go
> Reporter: Daniel Oliveira
> Assignee: Daniel Oliveira
> Priority: Minor
> Labels: triaged
> Time Spent: 40m
> Remaining Estimate: 0h
>
> Many error messages for common pipeline construction mistakes are unclear and
> unhelpful. They need to be improved to provide more context, especially for
> newer users. This bug tracks these error message improvements.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
