Test independent on Job class #4429

richtja · 2021-03-02T16:48:52Z

This commit removes the Job instance from the Test class. This is
preparation for legacy runner removal. For the nrunner doesn't make
sense to have Test dependent on Job instance, because Test is run inside
runner separated from Job.

Reference: #4425
Signed-off-by: Jan Richter jarichte@redhat.com

clebergnu · 2021-03-02T17:33:40Z

Hi @richtja , I've looked and tested the code, and this looks to be in the right direction. It will be a relief to finally get rid of the test -> job dependency.

Anyway, what we need here though, is a compatibility handling strategy. Basically, by changing the signature of the avocado.core.test.Test class, it impacts all other code that inherits from it. You've tackled the changes on this repo, but we have to consider external users too.

For instance, this would be needed to get Avocado-VT working with this change:

diff --git a/avocado_vt/test.py b/avocado_vt/test.py
index c518d8824..f6d147833 100644
--- a/avocado_vt/test.py
+++ b/avocado_vt/test.py
@@ -85,7 +85,7 @@ class VirtTest(test.Test):
     env_version = utils_env.get_env_version()
 
     def __init__(self, methodName='runTest', name=None, params=None,
-                 base_logdir=None, job=None, runner_queue=None,
+                 base_logdir=None, config=None, runner_queue=None,
                  vt_params=None):
         """
         :note: methodName, name, base_logdir, job and runner_queue params
@@ -112,7 +112,7 @@ class VirtTest(test.Test):
         self.background_errors.clear()
         super(VirtTest, self).__init__(methodName=methodName, name=name,
                                        params=params,
-                                       base_logdir=base_logdir, job=job,
+                                       base_logdir=base_logdir, config=config,
                                        runner_queue=runner_queue)
         self.builddir = os.path.join(self.workdir, 'backends',
                                      vt_params.get("vm_type", ""))

But then, it would only work with Avocado versions containing the change you're proposing. A better version of such a patch would handle the different signatures. I suggest you do the following next:

Work on the Avocado-VT compatibility
Add an Avocado-VT Cirrus CI jobs (just for the sake of the review process, a commit that will be throw away) that also uses Avocado containing the change you're proposing
Tell the community (a post to avocado-devel) about the upcoming change, and suggest the Avocado-VT change as a model for other extensions to keep compatibility with Avocado 86.0 and later.

This way, we'd make sure the compatibility is kept with all previous versions, and will be kept when your proposal (this PR) is merged.

avocado/core/test.py

richtja · 2021-03-03T10:07:11Z

But then, it would only work with Avocado versions containing the change you're proposing. A better version of such a patch would handle the different signatures. I suggest you do the following next:
1. Work on the Avocado-VT compatibility

2. Add an Avocado-VT Cirrus CI jobs (just for the sake of the review process, a commit that will be throw away) that also uses Avocado containing the change you're proposing

3. Tell the community (a post to avocado-devel) about the upcoming change, and suggest the Avocado-VT change as a model for other extensions to keep compatibility with Avocado 86.0 and later.
This way, we'd make sure the compatibility is kept with all previous versions, and will be kept when your proposal (this PR) is merged.

@clebergnu thanks for your suggestions. I will do that.

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>

pevogam

I have a few very superficial comments and questions regarding this PR, mostly things that arose from the VT-side PR.

avocado/core/test.py

pevogam · 2021-03-08T18:25:03Z

avocado/core/test.py

-        The job this test is associated with
-        """
-        return self.__job
-


Good riddance btw

pevogam · 2021-03-08T18:27:03Z

avocado/core/test.py

-            output_check_record = self.job.config.get('run.output_check_record')
-            output_check = self.job.config.get('run.output_check')
+        output_check_record = self.__config.get('run.output_check_record')
+        output_check = self.__config.get('run.output_check')


I assume self.__config will now always be checked? Simpler logic like this is definitely better.

pevogam · 2021-03-08T18:34:43Z

avocado/plugins/runner.py

@@ -92,6 +92,8 @@ def sigterm_handler(signum, frame):     # pylint: disable=W0613
            instance.set_runner_queue(queue)
        early_state = instance.get_state()
        early_state['early_status'] = True
+        early_state['job_logdir'] = job.logdir


Is this the new way to access the job logdir from a given test? It seems to me that the job is still very much involved in the test instances and its presence was changed from an attribute to a few keys of the mangled config attribute. I guess it is still better to get rid of the instance though.

Yes in the legacy runner the job is involved in the test instance. This PR is part of preparations for deprecating the legacy runner and make test class independent in nrunner. When we deprecate the legacy runner we will get rid of those.

pevogam

These changes are no doubt good, my comments are mostly meta.

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>

This commit removes the Job instance from the Test class. This is preparation for legacy runner removal. For the nrunner doesn't make sense to have Test dependent on Job instance, because Test is run inside runner separated from Job. Reference: avocado-framework#4425 Signed-off-by: Jan Richter <jarichte@redhat.com>

Signed-off-by: Jan Richter <jarichte@redhat.com>

codeclimate · 2021-03-09T15:07:02Z

Code Climate has analyzed commit aab4af7 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 47.7% (50% is the threshold).

This pull request will bring the total coverage in the repository to 70.3% (0.0% change).

View more on Code Climate.

beraldoleal

LGTM

clebergnu

LGTM, thanks!

richtja added the nrunner (previously nrun2run) label Mar 2, 2021

richtja added this to the #86 (Unknown) milestone Mar 2, 2021

richtja added this to Review Requested in Avocado Kanban via automation Mar 2, 2021

richtja linked an issue Mar 2, 2021 that may be closed by this pull request

Test independent on Job class #4425

Closed

beraldoleal reviewed Mar 2, 2021

View reviewed changes

avocado/core/test.py Show resolved Hide resolved

avocado/core/test.py Outdated Show resolved Hide resolved

richtja mentioned this pull request Mar 3, 2021

Future Test class independence on Job class in avocado avocado-framework/avocado-vt#2943

Merged

richtja force-pushed the independent_test branch from a800a3e to b4e6451 Compare March 5, 2021 10:18

pevogam reviewed Mar 8, 2021

View reviewed changes

pevogam approved these changes Mar 9, 2021

View reviewed changes

richtja added 2 commits March 9, 2021 15:29

Test cleanup method

aab4af7

Signed-off-by: Jan Richter <jarichte@redhat.com>

richtja force-pushed the independent_test branch from b4e6451 to aab4af7 Compare March 9, 2021 14:39

beraldoleal approved these changes Mar 11, 2021

View reviewed changes

clebergnu approved these changes Mar 11, 2021

View reviewed changes

clebergnu merged commit dceee1e into avocado-framework:master Mar 11, 2021

Avocado Kanban automation moved this from Review Requested to Done #86 (tbd) Mar 11, 2021

richtja deleted the independent_test branch July 13, 2022 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test independent on Job class #4429

Test independent on Job class #4429

richtja commented Mar 2, 2021

clebergnu commented Mar 2, 2021 •

edited

richtja commented Mar 3, 2021

pevogam left a comment

pevogam Mar 8, 2021

pevogam Mar 8, 2021

pevogam Mar 8, 2021

richtja Mar 9, 2021

pevogam left a comment

codeclimate bot commented Mar 9, 2021

beraldoleal left a comment

clebergnu left a comment

Test independent on Job class #4429

Test independent on Job class #4429

Conversation

richtja commented Mar 2, 2021

clebergnu commented Mar 2, 2021 • edited

richtja commented Mar 3, 2021

pevogam left a comment

Choose a reason for hiding this comment

pevogam Mar 8, 2021

Choose a reason for hiding this comment

pevogam Mar 8, 2021

Choose a reason for hiding this comment

pevogam Mar 8, 2021

Choose a reason for hiding this comment

richtja Mar 9, 2021

Choose a reason for hiding this comment

pevogam left a comment

Choose a reason for hiding this comment

codeclimate bot commented Mar 9, 2021

beraldoleal left a comment

Choose a reason for hiding this comment

clebergnu left a comment

Choose a reason for hiding this comment

clebergnu commented Mar 2, 2021 •

edited