Table driven tests for SimpleType, EnumType and ListType.

[pranavgore09]

Simple table driven testing of ConvertToModel for SimpleType, EnumType, ListType done.

Testing is never complete 😅 so removing label WIP.
We should be adding more and more tests on the go.

---

This change is

[tsmaeder]

Good stuff! Having the tests in place will help us as we extend the system.

---

Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

models/fieldtype_test.go, line 17 [r1] (raw file):

  t             SimpleType
  value         interface{}
  expectedValue interface{}

I would suggest that we add a check whether an error should be returnd. It is possible that 'nil' is actually a proper return value and not an error condition.

---

models/fieldtype_test.go, line 49 [r1] (raw file):

  for _, inp := range test_data {
      retVal, err := inp.t.ConvertToModel(inp.value)
      if retVal == inp.expectedValue {

I would suggest

if retVal == inp.expectedValue && (err != nil) == inp.errorExpected {
...

---

models/fieldtype_test.go, line 50 [r1] (raw file):

      retVal, err := inp.t.ConvertToModel(inp.value)
      if retVal == inp.expectedValue {
          fmt.Println("test pass:", inp)

I think t.Log() would be better, because it's controlled by the -v flag in "go test".

---

models/simple_type.go, line 34 [r1] (raw file):

      return value, nil
  case KindURL:
      if valueType.Kind() == reflect.String && govalidator.IsURL(value.(string)) == true {

It's already a boolean value, no need for the "== true"

---

models/simple_type.go, line 39 [r1] (raw file):

      return nil, fmt.Errorf("value %v should be %s, but is %s", value, "URL", valueType.Name())
  case KindFloat:
      // instant == milliseconds

the comment needs to be moved to the next case

---

Comments from Reviewable

[pranavgore09]

Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

models/fieldtype_test.go, line 17 [r1] (raw file):

Previously, tsmaeder (Thomas Mäder) wrote…

I would suggest that we add a check whether an error should be returnd. It is possible that 'nil' is actually a proper return value and not an error condition.

yes that is correct, nil can be real value. Will make the necessary change. also read other comments, will utilise in future commits. thanks.

---

models/fieldtype_test.go, line 49 [r1] (raw file):

Previously, tsmaeder (Thomas Mäder) wrote…

I would suggest

if retVal == inp.expectedValue && (err != nil) == inp.errorExpected {
...

yeah will update structure with errorExpected attribute

---

Comments from Reviewable

[baijum]

Variable declaration can be factored into blocks. e.g., https://tour.golang.org/basics/11

[tsmaeder]

---

Reviewed 3 of 4 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[kwk]

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

models/fieldtype_test.go, line 56 [r3] (raw file):
With the advent of Go 1.7 we might want to replace this

Quoted 9 lines of code…

for _, inp := range test_data {
      retVal, err := inp.t.ConvertToModel(inp.value)
      if retVal == inp.expectedValue && (err != nil) == inp.errorExpected {
          t.Log("test pass for input: ", inp)
      } else {
          t.Error(retVal, err)
          t.Fail()
      }
  }

with Subtests that use the new testing.T.Run() method:

func TestSimpleTypeConversion(t *testing.T) {
    t.Run("A=1", func(t *testing.T) { ... })
}

I'm not saying that we should do it now especially since there's not much setup code or tear down code. But having named tests that fail in a specific line is better than some loop where you have to have good output in order to deduce the actual test that was failing.

---

Comments from Reviewable

[kwk]

Reviewed 3 of 4 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable