Migrate Zeebe tests to certification tests #3112
Conversation
|
Converted this to a draft PR. Since the title still says "Work in progress". Additionally, please address @ItalyPaleAle's concern. |
| metadata: | ||
| name: zeebe-command | ||
| spec: | ||
| type: bindings.zeebe.command |
There was a problem hiding this comment.
Should this file be used as a conformance test or can a conformance test be added as well? Ideally, for the components we will want both explicit certification and conformance tests. Following the doc here, conformance tests would entail a tests/config/bindings/zeebe/bindings.yml.
There was a problem hiding this comment.
Components of type bindings contain a bit of everything, so conformance tests are often not much useful. If we have a good, comprehensive cert test, that's usually enough for a binding, especially for something that's more "unique" like Zeebe
There was a problem hiding this comment.
Oh, I see, so even though our certification lifecycle doc specified that for beta stage we have conformance tests and for alpha we have certification tests having it as is, and no explicit conformance tests, is sufficient for the beta stage?
There was a problem hiding this comment.
That document is correct, but we need to consider the specifics of each situation. A conformance test exists to ensure that all components behave similarly and can be used "interchangeably". This is very useful for things like state stores, secret stores, etc.
However, this binding has about a dozen operations that are unique to this component, so there's not much to "conform to".
Eventually, the decision to make this beta or stable will be at the maintainers' discretion.
There was a problem hiding this comment.
Great, thanks for explaining. I'm looking into the issues related to moving the zeebe command binding & zeebe jobworker binding to stable is why I was asking.
There was a problem hiding this comment.
@cicoyle Please, add this binding to conformance test to keep us following the process. I understand that we had discussions about how much value conformance tests can actually add to bindings, but I would prefer to keep the process until we reach a formal decision. A change to the certification process should go into proposals IMO since it is big enough.
There was a problem hiding this comment.
@artursouza adding a conformance test doesn't add value here since there isn't any standardized behavior and interface being tested in bindings. It just means taking up more GitHub actions runners.
I think this PR here is good enough to make the component stable. Also, I am surprised to find this discussion buried in comments on a PR from another author. That is not where this discussion belongs.
akkie
changed the title
|
@akkie Thanks for this contribution, so far looks good :) Could I ask one favor, please, to not force-push after we've done a review? When you force-push, GitHub features such as "changes since your last review" don't work anymore, so it makes it harder to review PRs (especially larger ones like this). We always squash-merge anyways, so no issues with keeping a lot of commits in the history. |
Signed-off-by: Christian Kaps <ck-github@mohiva.com>
Sorry, I usually squash my commits to easier rebase my changes on top of the latest master. Will not do it anymore. |
Signed-off-by: Bernd Verst <github@bernd.dev>
|
FYI @akkie I pushed two commits - one to enable the certification tests in CI (this is critical), and one to fix a linter issue! |
berndverst
dismissed
ItalyPaleAle’s stale review
Review feedback was addressed. Further changes can be made in follow up PR.
There was a problem hiding this comment.
There is a bug in unit tests now:
--- FAIL: TestInit (0.00s)
--- FAIL: TestInit/jobType_is_mandatory (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x94ea24]
goroutine 7 [running]:
testing.tRunner.func1.2({0x9d3340, 0xf1[76](https://github.com/dapr/components-contrib/actions/runs/6115916495/job/16600289608?pr=3112#step:9:77)00})
/opt/hostedtoolcache/go/1.20.7/x64/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
/opt/hostedtoolcache/go/1.20.7/x64/src/testing/testing.go:1529 +0x39f
panic({0x9d3340, 0xf17600})
/opt/hostedtoolcache/go/1.20.7/x64/src/runtime/panic.go:8[84](https://github.com/dapr/components-contrib/actions/runs/6115916495/job/16600289608?pr=3112#step:9:85) +0x213
github.com/dapr/components-contrib/bindings/zeebe/jobworker.(*ZeebeJobWorker).Close(0xb59ce0?)
/home/runner/work/components-contrib/components-contrib/bindings/zeebe/jobworker/jobworker.go:144 +0xe4
github.com/dapr/components-contrib/bindings/zeebe/jobworker.TestInit.func1(0x0?)
/home/runner/work/components-contrib/components-contrib/bindings/zeebe/jobworker/jobworker_test.go:61 +0x1a5
testing.tRunner(0xc0001f5520, 0xc000010[87](https://github.com/dapr/components-contrib/actions/runs/6115916495/job/16600289608?pr=3112#step:9:88)0)
/opt/hostedtoolcache/go/1.20.7/x64/src/testing/testing.go:1576 +0x10b
created by testing.(*T).Run
/opt/hostedtoolcache/go/1.20.7/x64/src/testing/testing.go:1629 +0x3ea
```
|
Running the tests on Mac instead I got: |
|
Please note that the contrib code freeze was technically weeks ago and this PR does change an existing component - not just tests. We can only include this PR if you can fix it up before we will be cutting the 1.12 release branch - which may be end of day tomorrow or on Monday. |
Signed-off-by: Christian Kaps <ck-github@mohiva.com>
Head branch was pushed to by a user without write access
|
@berndverst Fixed the issue |
Signed-off-by: Christian Kaps <ck-github@mohiva.com>
|
Can someone please approve the workflows again. There was a test failing |
Description
Converte Zeebe integration tests to Dapr certifications tests.
Fixes: #3111
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: