Support skip-tests option to skip some specific tests in alpha or beta features

[nak3]

Currently Alpha or Beta conformance tests must be implemented when we enabled the flag.
This patch allows users to skip one or some specific tests in alpha or beta features.

Fixes #183

[codecov]

Codecov Report

Merging #184 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #184      +/-   ##
==========================================
+ Coverage   94.45%   94.49%   +0.04%     
==========================================
  Files          34       35       +1     
  Lines         685      690       +5     
==========================================
+ Hits          647      652       +5     
  Misses         27       27              
  Partials       11       11              

Impacted Files	Coverage Δ
pkg/status/status.go	97.29% <0.00%> (ø)
pkg/testing/status/fake.go	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a71b40c...e21641f. Read the comment docs.

[dprotaso]

/lgtm

I'll add this requirement to my over-arching conformance improvements issue: knative/pkg#1563

Also go test -run is a bit clunky when skipping tests so I made this issue: golang/go#41583

[dprotaso]

It would be good to create a set of skipped test names and invoke t.Skip() so you get some output that a test was skipped

[nak3]

That is nice idea but t.Skip() calls against "root" test 😢. So It outputs SKIP: TestIngressConformance even though each conformance test outputs PASS like this:

--- SKIP: TestIngressConformance (0.00s)
    --- PASS: TestIngressConformance/headers/tags (8.02s)
        --- PASS: TestIngressConformance/headers/tags/matching_tag_header (1.29s)
        --- PASS: TestIngressConformance/headers/tags/no_tag_header (0.32s)
        --- PASS: TestIngressConformance/headers/tags/empty_tag_header (0.12s)
        --- PASS: TestIngressConformance/headers/tags/non-matching_tag_header (0.11s)

It is strange... So I think we should avoid to call t.Skip in run.go

[dprotaso]

You could artificially call skip on the subtest - ie.

if test.ServingFlags.EnableAlphaFeatures {
  for name, test := range alphaTests {
   if skipTest(name) {
    t.Run(name, func(t *t.testing.T) { t.Skip() }
    continue
  }
   t.Run(name, test)
  }
}

Would that give the desired output we want?

[coryrc]

Every test calls test.Setup(t), doesn't it? How about putting it in there?

[nak3]

Thank you. Updated. The current result shows like this:

=== RUN   TestIngressConformance/host-rewrite
    TestIngressConformance/host-rewrite: run.go:107: Skipping the test in skip-test flag

   ... 

--- PASS: TestIngressConformance (0.00s)
    --- SKIP: TestIngressConformance/host-rewrite (0.00s)
    --- PASS: TestIngressConformance/headers/tags (15.67s)
        --- PASS: TestIngressConformance/headers/tags/matching_tag_header (0.61s)
        --- PASS: TestIngressConformance/headers/tags/no_tag_header (0.11s)
        --- PASS: TestIngressConformance/headers/tags/empty_tag_header (0.10s)
        --- PASS: TestIngressConformance/headers/tags/non-matching_tag_header (0.21s)

[nak3]

@coryrc Thank you. But Setup is a function to setup test client. I feel it is a little bit weird that we put the Skip logic there. Let's go on Dave's suggestion.

[nak3]

/hold

@davetropeano Thank you for the review!
/cc @tcnghia @mattmoor @ZhiminXiang Could you also please take a look? knative-extensions/net-kourier#204 verified it.

[dprotaso]

/lgtm

leaving for others to review & unhold

[dprotaso]

cc @ZhiminXiang @JRBANCEL @tcnghia

[JRBANCEL]

It seems fine, albeit a bit convoluted and disappointing we can't rely on the standard Go constructs to do this.

[dprotaso]

It seems fine, albeit a bit convoluted and disappointing we can't rely on the standard Go constructs to do this.

I opened an issue - golang/go#41583 - give it some emojis ;)

[tcnghia]

/lgtm

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nak3, tcnghia

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:

• test/OWNERS [nak3,tcnghia]

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

[dprotaso]

/hold cancel