[test_runner] Prepare support for cleanup steps #118
Conversation
facebook-github-bot
added
the
CLA Signed
|
The In my opinion
(note that In this PR, I introduced the concept of Basically it's a Job descriptor extended with the information of steps descriptors, for all tests. The
Similar issue as above was affecting also the Among several other fields, the Together with these changes, we now have two constructors for
WARNING: one problem of this approach is that it breaks the backwards compatibility of |
Codecov Report
@@ Coverage Diff @@
## master #118 +/- ##
==========================================
- Coverage 61.50% 61.01% -0.50%
==========================================
Files 82 85 +3
Lines 3920 3937 +17
==========================================
- Hits 2411 2402 -9
- Misses 1219 1244 +25
- Partials 290 291 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| return runReporterBundles, finalReporterBundles, nil | ||
| } | ||
|
|
||
| func newBundlesFromSteps(descriptors []test.StepDescriptor, registry *pluginregistry.PluginRegistry) ([]test.StepBundle, error) { |
There was a problem hiding this comment.
Just a thought: may be newStepBundles is a more consistent name here? And the function below may be should be named something like newStepBundlesPair?
There was a problem hiding this comment.
Mh... the intention of this naming was to highlight that the bundles here are created from descriptors passed directly as part of the signature of the function, rather than fetched inside the function. I feel this information is lost with the naming suggested?
There was a problem hiding this comment.
Nah, it is clear from naming that it will construct bundles based on given steps. OK, makes sense. Though there's no such hint in the function below.
pkg/jobmanager/job.go
Outdated
| if err := td.Validate(); err != nil { | ||
| return nil, fmt.Errorf("could not validate test descriptor: %w", err) | ||
| } | ||
| bundlTargetManager, err := registry.NewTargetManagerBundle(&td) |
| TestStepLabel string | ||
| Parameters TestStepParameters | ||
| // StepsDescriptors bundles together Test and Cleanup descriptions | ||
| type StepsDescriptors struct { |
There was a problem hiding this comment.
All this entities are mixing-up in my head :)
If I understand correctly we usually use term Descriptor for serialized values. It confuses me a little-bit.
I need to draw a chart of these entities.
There was a problem hiding this comment.
Yeah, the intention since the introduction of these terms has been to use Descriptor as something that results from a deserialization, and Bundle as a collection of instances coming from instantiating objects based on the Descriptor.
|
Re-basing, then addressing all comments. |
pkg/test/step.go
Outdated
| Cleanup []StepDescriptor | ||
| } | ||
|
|
||
| // StepBundle contains the instantiation |
There was a problem hiding this comment.
Instantiation of what? I don't think I have understood what a bundle is yet :D
There was a problem hiding this comment.
Oh is this the golang datastructure version of StepDescriptor?
There was a problem hiding this comment.
I often pass out while writing comments. So, a Descriptor object is something on which we deserialize raw JSON. From a Descriptor, we build a Bundle, which holds the instances (e.g. of the Step) that were built from the Descriptor. I came up with this naming, but now I am not very happy about this naming, so I am definitely open for changing this (so for all the alternatives which came to mind are worse :/)
|
As far as I understood, StepDescriptor ([]test.StepsDescriptor) is actually, khm, part of job's state. Could it be named as <Cached|Resolved|Fetched|Prepared.. etc>StepDescriptor, just to reflect that it's an execution artefact, not input data? Also it would be handy to allow to read Events at least for Cleanup steps. (ie pass ev testevent.EmitterFetcher as a parameter to Run) I understand that it's pretty invasive and backward compatibility breaking change, but probably it's better to group such changes together.) |
|
@dimkup , you comment seems to be going in the same direction as @xaionaro comment (with respect to As for reading events, yeah, it would be probably necessary, though it needs more thinking. The idea of allowing test steps to read events was originally there, though it was reflected only on the |
|
We discussed that before landing this, we want to have a tool which allows for schema migration ready, and start versioning the db schema. It's going to take a while longer to get there. I am targeting next week to make it happen, as I really want to get it done with this stack of PRs). For the moment, I'll put this on hold. |
|
Resuming and rebasing this, after #154 was merged. |
|
Being superseded by #208, there are still some relevant changes in this PR which I will port to a different PR. |
This PR goes into two directions:
TestStephas been renamed into genericStep, as it now can represent both a test step and a cleanup step.TestDescriptorswhich was added toRequestobject in my opinion in not at the right level of abstraction.TestDescriptorsshould be inside theJobDescriptor, which is also included in theRequestobject.TestDescriptorsinto the database is obviously because we want to be able to rebuild the status of the job, even if the test description underneath has changed. But the PR that introduced the changes to persistTestDescriptorsin the database, did not add support for actually re-reading those descriptors when calculating job status. So the result is that when the status is calculated, we fetch again the description from the original source (which might have changed meanwhile)