Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial proposal for Testing Events #105

Closed
wants to merge 40 commits into from
Closed

Conversation

olensmar
Copy link
Contributor

Changes

Adds a "Testing Events" bucket with corresponding subjects and predicates.
Fixes a bunch of broken links and some grammar in existing docs.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Copy link
Contributor

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your contribution to test case events, it's really welcome!
I have not completed the review yet, but I have a few initial comments that I wanted to share, let me know what you think.

schemas/testcasefinished.json Outdated Show resolved Hide resolved
schemas/testcasefinished.json Outdated Show resolved Hide resolved
schemas/testcasefinished.json Outdated Show resolved Hide resolved
@afrittoli
Copy link
Contributor

As a side note, once this merges we will have a new bucket of events, so we will need to update the website accordingly https://github.com/cdevents/cdevents.dev

@olensmar
Copy link
Contributor Author

olensmar commented Jan 31, 2023

@afrittoli @TheBrunoLopes I've made a bunch of updated in line with yesterdays discussion:

  • changed to testSuiteRun/testCaseRun
  • added queued events
  • changed version to 0.1.0-draft
  • introduces objects for environment and trigger
  • added note on deprecation of old subjects
  • updated schemas accordingly

Copy link
Contributor

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, I think it looks good!
I haven't reviewed the JSON schemas in details yet, but I think the data model is ok.

A question about the tables, for the other events the subject tables include all possible subject attribute, while the event table show which are relevant to the specific predicate.
In the proposed testing-events.md the tables of subjects and predicate only intersect on the id/source tuple - is that on purpose? Does this mean that all other attributes are optional?

I'm not sure if the current format for the tables is the most readable, we did a couple of iterations but it may still be not very obvious. Do you have any recommendation on how to improve it?

README.md Outdated Show resolved Hide resolved
testing-events.md Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
@olensmar
Copy link
Contributor Author

olensmar commented Feb 9, 2023

Thanks for the updates, I think it looks good! I haven't reviewed the JSON schemas in details yet, but I think the data model is ok.

A question about the tables, for the other events the subject tables include all possible subject attribute, while the event table show which are relevant to the specific predicate. In the proposed testing-events.md the tables of subjects and predicate only intersect on the id/source tuple - is that on purpose? Does this mean that all other attributes are optional?

I'm not sure if the current format for the tables is the most readable, we did a couple of iterations but it may still be not very obvious. Do you have any recommendation on how to improve it?

I was actually a bit confused on this - thanks for this explanation. Initially I only added any additional predicate-specific properties to the predicate tables (apart from id/source) - i'll adjust so that the predicate tables contain all applicable subject-properties + additional predicate-specific properties

@olensmar olensmar marked this pull request as ready for review February 9, 2023 11:29
@olensmar olensmar requested review from xibz, afrittoli and TheBrunoLopes and removed request for xibz, afrittoli and TheBrunoLopes February 9, 2023 11:29
spec.md Outdated Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
@afrittoli
Copy link
Contributor

This will have to be updated based on #116 and possibly #118 (depending on which PR merges first).

@afrittoli
Copy link
Contributor

@olensmar I'm going to start preparing v0.2 spec release today - would it be ok to schedule this for v0.3?

@olensmar
Copy link
Contributor Author

@afrittoli yes - no problem - i'll fix the merge errors asap

@afrittoli afrittoli added this to the v0.3 milestone Mar 27, 2023
@olensmar
Copy link
Contributor Author

olensmar commented Apr 4, 2023

thanks again @e-backmark-ericsson - all comments should have been handled now...

Copy link
Contributor

@e-backmark-ericsson e-backmark-ericsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for adding a lot more comments to this review... I don't want to delay the introduction of these new testing events, so if you're fine with having quite substantial changes to them in later versions of the protocol then I'm ok with merging this PR after just some minor versioning glitches and such.

@afrittoli , please provide your view on how stable and how well thought through these events need to be in order to be merged.

@@ -0,0 +1,28 @@
{
"context": {
"version": "0.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema release we're now aiming at is 0.3.0, and therefore this version field should state "0.3.0-draft". The same comment is valid for all example files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec version is 0.3.0, the event version is 0.1.0, so:

.context.version should be set to 0.3.0-draft
.context.type should be reverted to dev.cdevents.testsuiterun.finished.0.1.0-draft

schemas/testcaserunfinished.json Outdated Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
schemas/testcaserunfinished.json Outdated Show resolved Hide resolved
schemas/testcaserunfinished.json Show resolved Hide resolved
schemas/testcaserunfinished.json Show resolved Hide resolved
testing-events.md Show resolved Hide resolved
@afrittoli
Copy link
Contributor

afrittoli commented Apr 12, 2023

I'm able to run the SDK generation tool with the latest schemas, even though I will need some updates to the generation logic to support objects other than references (like the testCase) cdevents/sdk-go#51

},
"type": {
"type": "string",
"enum": [
"dev.cdevents.testcase.finished.0.1.1"
"dev.cdevents.testoutput.published.0.3.0-draft"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The event version here should be 0.1.0-draft - see the versioning docs for reference https://cdevents.dev/docs/primer/#versioning

olensmar and others added 6 commits April 18, 2023 13:41
fixed enum example

Co-authored-by: Emil Bäckmark <emil.backmark@ericsson.com>
# Conflicts:
#	examples/testcase_finished.json
#	examples/testcase_queued.json
#	examples/testcase_started.json
#	examples/testsuite_finished.json
#	examples/testsuite_started.json
#	schemas/testcasequeued.json
#	schemas/testcasestarted.json
#	schemas/testoutputpublished.json
#	schemas/testsuitefinished.json
#	schemas/testsuitestarted.json
@olensmar
Copy link
Contributor Author

@afrittoli @e-backmark-ericsson thanks for your hard work with this - I think we're all set!?

Copy link
Contributor

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Ole!

| environment | `Object` [`environment`](continuous-deployment.md/#environment) | The environment in which this testCaseRun is queued | `{"id": "1234"}`, `{"id": "dev", "source": "testkube-dev-123"}` | ✅ |
| testCase | `Object` [`testCase`](#testcase) | Definition of the testCase being executed | `{"id": "92834723894", "name": "Login Test", "type": "integration"}` | |
| testSuiteRun | `Object` [`testSuiteRun`](#testsuiterun) | A testSuiteRun to associate this testCaseRun with a containing testSuiteRun | `{"id":"Auth-TestSuite-execution-12334", "source": "staging/testkube"}` | |
| trigger | `Object` [trigger](#trigger) | What triggered this testSuiteRun | `{"type": "schedule"}` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is trigger actually needed? I ask since I am currently working on the links proposal, so that would be able to describe what triggered it. So may be odd to have a field that says the same thing.

@afrittoli thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm.. I think understanding how/why a test is run can be helpful in deciding on how to handle it's outcome - was it run manually? triggered by a deployment? scheduled? How would the links proposal play into this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xibz What do you think about implementing triggers for events now - if at some point we see that the connecting events functionality can provide the same value/functionality, we could remove them then.

| Field | Type | Description | Examples | Required |
|---------|-----------------|-----------------------------------------------------------------------------------------------------------------------|----------------------------|----------|
| id | `String` | Uniquely identifies the testCase within a test management system. | `12312334` | ✅ |
| type | `String enum` | The type of test, one of `performance`, `functional`, `unit`, `security`, `compliance`, `integration`, `e2e`, `other` | `functional` | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the benefit of having type? This seems like a place for ambiguity and confusion especially when parties have different definition of a test type. I've seen various different definitions for functional and unit. Various definitions for integration. Unless all tools use the same definition, it seems like a place where tools can potentially get mixed up on unless we are very *specific on these terms. I just want to understand it's purpose before I write it off, but I am leaning towards not including it tbh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's ambiguous, but I think it can still be helpful on the receiving side to have some idea of what kind of tests that are being run - for grouping/filtering/notification purposes perhaps!? The property is optional - and we'll see if/how implementers choose to support it going forward - so I don't see any risks with having it here to start with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xibz We discussed this previously on the PR and in the working group, we are not sure whether this is the best way to implement the type either but we'd like to keep it as a starting point because it would be beneficial for event consumers especially. Producers that run tests that do not fit any of the other categories may decide to use other or skip the field completely.
We can add additional values to the enum in future if needed.

A `testSuite` is a collection of `testCase` objects managed in an external system. Each time a `testSuite` is executed the
corresponding `testSuiteXXX` events should be emmitted.

| Field | Type | Description | Examples | Required |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice there's no list of test cases ids, names, or anything in here? This could be solved with what I am working on via links, but it does seem odd to have test suite with the definition being a list of test cases, and no array of test cases in its model.

What is the flow of this event?

CI test trigger -> test suite #1 -> test case #1  -> test case #2 -> test case #3

Test cases 1-3 would have test suite 1 as its parent. Is that the idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes you're right - that's the idea - idk how much of the actual test-related models we want/need to expose in these events - trying to keep it down to a minimum for now

Copy link
Contributor

@xibz xibz Apr 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, just want to make sure I am understanding that right. One thing is still not making sense, however.

Let's break it down into a real world situation:
Let's assume we have a user, a CI system (let's use jenkins), CD system (spinnaker).
The CI and CD system use an event bus for cdEvents.

  1. The user submits a PR to github, which is a triggers CI
  2. Jenkins will run a series of commands provided by the user from some file in their repository.
  3. Jenkins usually runs a test suite, but it may not be, as it is totally defined by the user.
  4. Test suite than calls individual tests written by the user
  5. Test suite passes
  6. PR is merged
  7. Artifacts are published to some artifact store
  8. Spinnaker than received an artifact event
  9. etc...

I see a few issues here. One jenkins will not know what's a test case, test suite, or what the user is even providing; only the user knows. How do we plan to solve for that?

Jenkins runs a test suite which waits for the test cases to finish (written by the user), then submits the test suite event back to the bus. This test suite will need to have pass or fail or even skipped as a state. Further, the more bigger problem, is test cases are written by users. How are we going to get cdEvents of test cases to the event bus? This seems does not seem like a good idea, and I feel over complicates everything.

My proposal is we do away with test case, and just have test suite. Test results instead will point to some test suite, which solves the missing state issue, otherwise imagine github needing to listen for n number of test cases to figure out what passed, what was ran, etc.

Im still unsure on how Jenkins will know that the command the user provided is a test, but let's solve these first few problems first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @xibz for this - great input!

I definitely agree that most build tools do not have detailed insights into the actual tests being run - but this proposal is not meant to align with that shortcoming (as you describe for Jenkins). The purpose here has been to define test-related subjects and predicates that provide "first-class" insights into testing-related activities/events as part of any CD pipeline - be it CI/CD/GitOps/etc - hopefully resulting into more QA-attuned workflows and deployments.

From an implementation PoV the tool(s) that actually run the tests (and not the build tool that triggers them) should be expected to emit these events; in your specific example I think it would be sufficient for Jenkins to emit testsuite-related events (queued, started, finished) - while the actual test-tool-runner could emit more detailed events on specific test-cases/types, test output, etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this in the WG, @e-backmark-ericsson and myself agree that this is a good starting model - where we assume that it's the test tool's responsibility to send test-related events, i.e. Jenkins, Tekton and other pipeline orchestrators are not expected to send these events.

The connected-events work could have an impact on this later on - which we can deal with in future updates of these events.

Copy link
Contributor

@xibz xibz Apr 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A testing library is any arbitrary code. So unless you change how a testing library authenticates (which none exists today), you're going to allow anyone to send any sort of event, and even worse they can just send anything to the event bus whether it's properly formatted events or not. That's a huge security concern.

I think it's important to understand what is a useful event vs not. No system today is looking for a particular test case. Not even QA aware. All they care about is test suites, and a test suite can be a single test case. I am at least unaware of any system, and wouldn't even understand why they'd care. I just dont see any benefit to this event, and with it's security implications it becomes a hard no from me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @xibz - There are plenty of testing solutions/tools out there that model testing into testsuites and testcases - and emitting corresponding events to consumers will give more flexibility on how to react to these accordingly. What ultimately makes a "useful" event is up to the user to decide, not us, and I don't see a reason not to provide this granularity if it can help users build better workflows or pipelines.

Also these security concerns don't make any sense to me - any system/tool/framework that supports CDevents could potentially be an emitter of "malicious" events, be they authenticated or not - @afrittoli perhaps you can provide some guidelines in this regard? Are there defined "constraints" on who/what is allowed to emit events in a given context?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An important mission of the CDEvents spec is to bring standardisation and interoperability to the CI/CD space, which I believe the test events will help achieve. We have not yet discussed the topic of trust in the event sources and event contents, even though that's definitely something we want to address. I do not see a specific issue in that area with test events though - @xibz could you elaborate further about your concerns?

I do not think it's likely that testing libraries like pytest or the go test command will start sending test events directly to consumers - but it is possible for CDEvents to drive standardisation in the dictionary and structures such libraries adopt.
Test management tools are well-positioned to send test events - whether test suite or test case related depends on the granularity we need for automation and data collection for metrics.

We do have test case and test suite events today in CDEvents, what this PR does is to refine their data model and adapt the names to reflect the focus on test execution rather than definition.

Copy link
Contributor

@xibz xibz Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An important mission of the CDEvents spec is to bring standardisation and interoperability to the CI/CD space

100%. So why is test cases important when no CI/CD tool cares about a test case?

could you elaborate further about your concerns?

Yea, so you are moving who has authority to send data to the event bus. Before it was internal in the CI/CD system space, but once you introduce tooling that can send to it from user space, that opens a whole bunch of security concerns, and honestly the wrong way to go about it. Let's break down how a lot CI systems "tests" something.

  1. A container get's spun up based usually on some docker image
  2. That docker image installs/bootstraps whatever is needed to execute some command
  3. For example, to install pytest, they'd run pip install pytest
  4. This pytest would need some way of sending from out of the container to this event bus (security concern 1)
  5. What stops a user from installing something like pytest except malicious that sends whatever to this event bus?

Things like github actions, etc, have no way of vetting this process without going to extremes to lock down dependencies, which is not an experience any CI tool wants. That's the point of containers or VMs. To give that security layer. However, this test case being sent from pytest, just opens up that door in allowing for any tool, not just pytest, to send to an event bus.

Before you only needed to trust the CI and CD systems. Now you have to trust users. Yea, that's not going to work. Due to these concerns, I feel it is better to leave out test cases and just rely on test suites instead and a test case is just a test suite with a single test. You avoid needing to think about security in this way, and it simplifies things a lot more. The biggest take away from this whole discussion is the want to solve for every single problem (big or small) which I dont think is a good way to think about it. I think we should have the spec/protocol be flexible for all systems. Test suites allow for that. What use cases where you absolutely need test cases which test suites cant do a good of a job?

pytest wont send directly to consumers

Why would any testing framework need to now send a cdEvent to somewhere? That seems way out of scope for a testing library.

To summarize everything, is test cases dont seem useful. I know you mentioned metrics, but we are solving for interoperability. For me, I just dont see how any system will be able to utilize test cases without massive changes to the system, and why would you make changes for extra unneeded granularity. I would much rather start with test suites and let the market drive what is needed. If people start asking for test suites then we should 100% support it. I think it's just too early to tell and without making massive changes to existing CI systems or having testing libraries send the events, test cases wont be too useful.

Copy link
Contributor

@xibz xibz Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edit: I believe we were talking about two different things. When you guys were saying test tools, you didnt mean local testing tools, but testing services (AKA part of CI :p). So that's where the confusion was.

@xibz
Copy link
Contributor

xibz commented Apr 22, 2023

This looks good to me. I just had a few questions, and some feedback.

@e-backmark-ericsson
Copy link
Contributor

Thanks for your comments @xibz ! Given the answers from @olensmar and @afrittoli , are you ok with this PR being merged now (once the commit is signed)?

@xibz
Copy link
Contributor

xibz commented Apr 25, 2023

@e-backmark-ericsson yea, this looks good!

@olensmar olensmar deleted the branch cdevents:main April 26, 2023 12:43
@olensmar olensmar closed this Apr 26, 2023
@olensmar olensmar deleted the main branch April 26, 2023 12:43
@olensmar olensmar mentioned this pull request Apr 26, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants