Add unit tests for 'basic_no_decorator.py' and 'compose.py'

[drewbutlerbb4]

Resolves #107

Adds functions to help with unit tests for general nested pipelines and pipelines without decorators. Adds unit tests specifically for "basic_no_decorator.py" and "compose.py".

Also addresses some nits from #105

[kubeflow-bot]

This change is 

[drewbutlerbb4]

I will update test_kfp_sample.sh once #110 has merged

[ckadner]

@drewbutlerbb4 -- #110 has merged, please rebase

[drewbutlerbb4]

test_kfp_samples.sh has been updated. Thanks for the suggestion to restructure the tests, @ckadner, I think this looks a lot cleaner.

[ckadner]

Thanks @drewbutlerbb4 -- this looks good, but would you be able to also address the second part of point number 3 mentioned in #107?

• change the code in test_util.py so that it calls on the new unit test cases and make sure to generate/copy the "golden" YAML and copy them to the specified output path

... to make sure we get the right exit code AND the compiled output YAML -- similar to the parameters of dsl-compile --py <DSL script> --output <YAML file>

see comment :

wrap the compile part into a bash function and inside that function check for IS_SPECIAL:

function compile_dsl {
  if [ -z "${IS_SPECIAL}" ]; then
    dsl-compile-tekton --py "$1" --output "$2"
  else
    python3 -m test_util $1 ${CONFIG_FILE} # need to produce YAML file output
  fi
} 

# ...

    if compile_dsl "${f}" "${TEKTON_COMPILED_YAML_DIR}/${f##*/}.yaml" >> "${COMPILER_OUTPUTS_FILE}" 2>&1 ;
    then
      echo "SUCCESS: ${f##*/}" | tee -a "${COMPILER_OUTPUTS_FILE}"
    else
      echo "FAILURE: ${f##*/}" | tee -a "${COMPILER_OUTPUTS_FILE}"
    fi

If not in this PR, maybe in a follow up? I need that change to restore how the test_kfp_samples.sh script works, to capture the compiler console output as well as exit code and the produced YAML.

[drewbutlerbb4]

@ckadner The least intrusive way of doing this, that I can think of, is to add an optional parameter save_path to TestTektonCompiler method's _test_nested_workflow and _test_workflow_without_decorator. Then call these methods from two new TestTektonCompiler method test_compose_with_save and test_no_decorator_with_save that test_util will call. Is this an acceptable structure or did you have something else in mind?

[drewbutlerbb4]

On second thought we could add an optional parameter save_path to test_basic_no_decorator and test_compose rather than creating two entirely new methods

[ckadner]

On second thought we could add an optional parameter save_path to test_basic_no_decorator and test_compose rather than creating two entirely new methods

Actually, you could just use the "golden" YAML if the test succeeds and store it in the specified path. No need to edit the unit test case. If it fails you should exit with exit code 1 anyway (i.e. raise an Error).

[drewbutlerbb4]

What about in the case where the pipeline compiles but doesn't match the "golden"? Wouldn't the unit test fail and we would record a FAILURE rather than a SUCCESS because it compiled?

[ckadner]

What about in the case where the pipeline compiles but doesn't match the "golden"? Wouldn't the unit test fail and we would record a FAILURE rather than a SUCCESS because it compiled?

If the pipeline compiles but does not match the "golden" YAML, then the unit test should fail. If the change in the output YAML is desired, then the "golden" YAML needs to be regenerated (i.e. by flipping the GENERATE_GOLDEN_YAML switch in compiler_tests.py)

The behavior of test_util.py (before this PR) to print out SUCCESS or FAILURE is what I want to get rid of. Instead the test_util.py Python process should return exit code 0 (normal completion) and produce the "golden" YAML file in the specified location, or, if the unit test failed, just exit with an error (process exit code 1) -- which emulates the behavior of dsl-compile-tekton.

[drewbutlerbb4]

Thanks for the clarification @ckadner, amended based on your comments

[ckadner]

Thank you @drewbutlerbb4 -- this looks good. Just a few nitpicks and small code refactor request

[ckadner]

Thanks @drewbutlerbb4 -- this looks good. I only have little nitpicks :-)

[drewbutlerbb4]

Thanks for the comments @ckadner, amended.

[ckadner]

Great. Thanks @drewbutlerbb4

/lgtm

[ckadner]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ckadner

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ckadner]

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