Future Test class independence on Job class in avocado #2943
Conversation
There was a problem hiding this comment.
LGTM, extended testing with the proposed change in the Avocado side can be seen here:
https://cirrus-ci.com/build/6148588276285440
More specifically in jobs such as:
https://cirrus-ci.com/task/5809965270040576
https://cirrus-ci.com/task/5247015316619264
|
Hi @pevogam, this is the part of work on avcado-vt compatibility with nrunner. It's connected with avocado #4429. I am letting you know as you wanted to be part of this effort. |
There was a problem hiding this comment.
Hi @richtja, thanks for including me here. As this PR is touching code job/test API it actually had breaking changes for our plugin and environment so it was good to take a look at it while simultaneously doing a review and fixing our own calls.
I left a few comments for you here, tests seem to be passing now so unless I write with more comments our tests can be considered green.
avocado_vt/test.py
Outdated
| """ | ||
| :note: methodName, name, base_logdir, job and runner_queue params | ||
| :note: methodName, name, base_logdir, job/config and runner_queue params |
There was a problem hiding this comment.
Perhaps this could be simply named config here? It is confusing what is meant with job/config and it seems the job keyword is dropped.
There was a problem hiding this comment.
This PR is about to make avocado-vt compatible with new version of avocado where Test class uses config, but avocado-vt still have to be compatible with avocado LTS version where Test class uses job object. I understand that this might be confusing, so I will write better docstring with explanation.
There was a problem hiding this comment.
Oh I see. Yes, this is a good idea.
| are inherited from test.Test | ||
| :param params: avocado/multiplexer params stored as | ||
| `self.avocado_params`. | ||
| :param vt_params: avocado-vt/cartesian_config params stored as | ||
| `self.params`. | ||
| """ | ||
| vt_params = kwargs.pop("vt_params", None) |
There was a problem hiding this comment.
What is the planned difference between vt_params and params_vt? I think the presence of both could be confusing for people and perhaps it is best to provide a single instance of the Params class for all usage within VT.
There was a problem hiding this comment.
I just follow the naming from the master branch. I agree with you that this can be change, but I think it is not related to this PR. And because I am planing to use this as an example, for other extensions to keep compatibility with Avocado 86.0 and later, to the community, so I would like to make it as simple as possible.
There was a problem hiding this comment.
Yes no worries, I was suspecting that and I would do the same. I was rather asking if you know more about these and if my comment makes sense to you for a future PR. In any case it is something to consider.
In the avocado 86.0 will be removed the test -> job dependency. This commit will ensure the compatibility of avocado-vt with avocado>=86 Reference:avocado-framework/avocado#4429 Signed-off-by: Jan Richter <jarichte@redhat.com>
richtja
force-pushed
the
independent_test
branch
from
ca46d24
to
1014be2
Compare
In the avocado 86.0 will be removed the test -> job dependency. This
commit will ensure the compatibility of avocado-vt with avocado>=86
Reference:avocado-framework/avocado#4429
Signed-off-by: Jan Richter jarichte@redhat.com