First stage of new tests which share logic with library tests

[mmulholla]

This is part1 of replacing the existing api tests and is a work in progress. The idea is for the library test and api test to use the same code for generating valid devfiles which will exist in the api repo. The code is not complete yet but the current set of tests run. The library tests depend on this PR before they can be merged.

Things to do include, but are not limited to:

• Expand tests to fully cover the schema.
• Expand test README with more detail on how the tests work and how to update them.
• Come up with a way to configure which schema to use.

For more information see: #347

[yangcao77]

perhaps function WriteAndValidate and writeDevfile can be under pkg test/v200/utils/common?
So you do not need to define the empty struct DevfileValidator to avoid import cycle

[mmulholla]

The method implementations are completely different comparing the api and library implementations so they do not belong in the common. This is the standard way of implementing an interface declared in a different package.

[maysunfaisal]

This is confusing and had to back track a couple of times to follow it, we are calling an interface implementation for validator but the func takes its own entire struct?

I am not sure if this is a clean way to do it in Go.

[mmulholla]

I am all ears for a better way to do it.

[amisevsk]

I looked briefly at the code but didn't easily find definitions for the various structs/interfaces used (i.e. where does Validator come from), but the way to do it would be to make TestDevfile implement the Validator interface:

type Validator interface {
  WriteAndValidate() error
}

type TestDevfile struct {
  myField string
  mySetting bool
}

func (t *TestDevfile) WriteAndValidate() error {
  t.myField = "write"
  if !t.mySetting {
    return errors.New("Bad setting")
  }
}

// Now `TestDevfile` can be used wherever a `DevfileValidator` is needed

[maysunfaisal]

In common/test_utils.go, you can update:

// DevfileValidator interface implemented by the parser and api tests for verifying generated devfiles
type DevfileValidator interface {
	WriteAndValidate(*TestDevfile, DevfileFollower) error
}

// Runs a test to create and verify a devfile based on the content of the specified TestContent
func (testDevfile *TestDevfile) RunTest(testContent TestContent, t *testing.T, follower DevfileFollower, validator DevfileValidator) {

...

err := validator.WriteAndValidate(testDevfile, follower)
	if err != nil {
		t.Fatalf(LogErrorMessage(fmt.Sprintf("ERROR verifying devfile :  %s : %v", testContent.FileName, err)))
	}

}

in api/test_utils.go, you can update:

// WriteAndValidate implements Saved.DevfileValidator interface.
// writes to disk and validates the specified devfile
func (devfileValidator DevfileValidator) WriteAndValidate(devfile *commonUtils.TestDevfile, follower commonUtils.DevfileFollower) error {
	err := writeDevfile(devfile)
	
	...
}


// RunTest : Runs a test to create and verify a devfile based on the content of the specified TestContent
func RunTest(testContent commonUtils.TestContent, t *testing.T) {

      ...

       testDevfile.RunTest(testContent, t, nil, validator)
}

That way you dont have to embed validator and follower in TestDevfile and its much easier to read and track

[mmulholla]

I don't understand how your versions are not much uglier than mine. The follower is needed in the common code before the validator. Add the interface to an existing structure is better than a specific obviously named structure?

[mmulholla]

Understand this is a test, it is meant to exercise the library api's it is not meant to be the most efficient, some places it goes out of its way to pound the api . The common code informs the follower every time something is added to the devfile, which will end up as twice for each property added to schema, for example once to add an empty command and a second to update it with attributes. This is only needed for the parser. Once the entire devfile is created it needs to be written out and validated, which is different for the parser and the api hence the validator. This set up means the api side does not need and empty follower. This is. the pain of having common code.

[yangcao77]

simplify the message

Suggested change

	common.LogErrorMessage(fmt.Sprintf(" FAIL : %s : schema : failed to convert to json : %v", devfile, err))
	common.LogErrorMessage(fmt.Sprintf(" FAIL : schema : failed to convert %s to json : %v", devfile, err))

[mmulholla]

I prefer my format.

[yangcao77]

fine. it's up to you

[mmulholla]

@maysunfaisal @yangcao77. Thanks for the comments would like to address term in my next PR if OK, nothing functional in the comments that would affect outcomes.

On a note the idea is that common code is common to both sides and is agnostic to which side is using it. To make this possible I added two interfaces in the common code for the api and library components to implement as necessary. Both need to implement the DevfileValidator and just the library needs to implement DevfileFollower. How this is then implemented is pretty standard for go - given the interface is defined in a different package to the implementor. If this is not what you expected please shout now.

One impact of this is that of this is that TestDevfile is now in common code but used in the api and library sides. In common code I can implement functions for it but I cannot in api and library so I had to modify some methods in api and library to take it as a parameter. If it was Java would have used "extends" but not sure that can be done in go. Would be interested if there is a better way. but for now I think this PR is good to go.

[mmulholla]

@maysunfaisal @yangcao77 have now added a commit for all the things I planned to do in the next PR.

[yangcao77]

I'm still feeling wired of this:

api/test/v200/utils/api/test_utils.go

Lines 30 to 34 in 8ae84fb

	type DevfileValidator struct{}
	// WriteAndVerify implements Saved.DevfileValidator interface.
	// writes to disk and validates the specified devfile
	func (devfileValidator DevfileValidator) WriteAndValidate(devfile *common.TestDevfile) error {

The empty struct DevfileValidator and the receiver function func (devfileValidator DevfileValidator) WriteAndValidate

I understand this is trying to resolve the cycle import issue, but it is very confusing when reading the code. Need to figure out a better way to make it more clear. But we can do it in the next PR.

My concerns have been resolved, just a minor typo as pointed out.
Waiting for Maysun's response as he also put some review comments

[mmulholla]

Have pushed more changes for the comments I missed.

For info on the interface structure I worked from this: https://github.com/golang/go/wiki/CodeReviewComments#interfaces

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: maysunfaisal, mmulholla
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment