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.

Sign up for GitHub

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

Jump to bottom

Requirements database [v2] #5418

Closed
wants to merge 6 commits into from
Closed

Requirements database [v2] #5418

wants to merge 6 commits into from

Conversation

richtja
Copy link
Contributor

@richtja richtja commented Jun 29, 2022

This PR adds the ability to spawners to store requirements into cache.
This will make more efficient running multiple tests with the same
requirement or rerunning the jobs itself.

This also brings ability to use requirements with podman spawner. So now
is possible to run something like this:

class SuccessTest(Test):

    def check_hello(self):
        result = process.run("hello", ignore_status=True)
        self.assertEqual(result.exit_status, 0)
        self.assertIn('Hello, world!', result.stdout_text,)

    def test_a(self):
        """
        avocado dependency={"type": "package", "name": "hello"}
        """
        self.check_hello()

Warning: Right now, the requirement cache doesn't have any controls for
clearing cache or updating, and also didn't check the current
environment. So if the user do some changes to the environment outside
Avocado runtime, like uninstalling package, podman prune, removing
asset, etc. The tests with requirements will be broken.

Signed-off-by: Jan Richter [email protected]

Changes from v1 (#5412):

  • Typo fixes.
  • Usage of list comprehensions.
  • New and well-defined RuntimeTask states.
  • Usage of hostname in cache for process spawner.
  • Change test_requirements.py to avocado Test and usage of varianter.
  • Documentation update.

@richtja richtja added this to the #98 milestone Jun 29, 2022
@richtja richtja requested review from beraldoleal and clebergnu June 29, 2022 15:58
@richtja richtja self-assigned this Jun 29, 2022
@richtja richtja mentioned this pull request Jun 29, 2022
Closed
@richtja richtja force-pushed the requirements_database_v2 branch from 538aaf7 to a6b407c Compare June 30, 2022 11:10
clebergnu
clebergnu requested changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

Hi @richtja,

Thanks for following up with this v2. This version looks pretty close to getting my ACK, but there are a small few things that need your attention.

avocado/core/task/runtime.py Outdated Show resolved Hide resolved
docs/source/guides/user/chapters/requirements.rst Outdated Show resolved Hide resolved
docs/source/guides/user/chapters/requirements.rst Outdated Show resolved Hide resolved
selftests/functional/serial/test_requirements.py Outdated Show resolved Hide resolved
selftests/check.py Outdated Show resolved Hide resolved
avocado/core/task/runtime.py Outdated Show resolved Hide resolved
richtja added 6 commits July 1, 2022 11:17
@richtja
Run pre-test task in serial way
0c10ee8 
It changes order of runtime tasks in dependency graph, in such way that
pre-test task are depending on each other, which ensures serial run in
state machine for pre-test tasks. It will solve problems with conflicts
between different pre-test task runs.

Signed-off-by: Jan Richter <[email protected]>
@richtja
List finished dependencies
063cd61 
The runtime task has information about all its dependencies. Let's add a
method which will list all of them which have been already finished.

Signed-off-by: Jan Richter <[email protected]>
@richtja
move check_finished_dependencies to RuntimeTask
52e105e 
The RuntimeTask has all the information to about dependencies to make a
decision if the dependencies were resolved, and it is possible to run a
task. So let's move this responsibility to the RuntimeTask.

Signed-off-by: Jan Richter <[email protected]>
@richtja
Requirements cache database updates
3cfbbb8 
This adds few methods for manipulating with requirements cache. This
will be useful for spawners which will use requirements cache.

Signed-off-by: Jan Richter <[email protected]>
@richtja
Requirements cache
89def40 
This commit add ability to spawners to store requirements into cache.
This will make more efficient running multiple tests with the same
requirement or rerunning the jobs itself.

This also brings ability to use requirements with podman spawner. So now
is possible to run something like this:

class SuccessTest(Test):

    def check_hello(self):
        result = process.run("hello", ignore_status=True)
        self.assertEqual(result.exit_status, 0)
        self.assertIn('Hello, world!', result.stdout_text,)

    def test_a(self):
        """
        🥑 dependency={"type": "package", "name": "hello"}
        """
        self.check_hello()

Warning: Right now, the requirement cache doesn't have any controls for
clearing cache or updating, and also didn't check the current
environment. So if the user do some changes to the environment outside
Avocado runtime, like uninstalling package, podman prune, removing
asset, etc. The tests with requirements will be broken.

Reference:
Signed-off-by: Jan Richter <[email protected]>
@richtja
Runtime task statuses
6e689d2 
This will define the values for `RuntimeTask.status` so it will be
easier to use them, and It will be easier to detect in which state the
`RuntimeTask` is.

Signed-off-by: Jan Richter <[email protected]>
@richtja richtja force-pushed the requirements_database_v2 branch from a6b407c to 6e689d2 Compare July 1, 2022 09:49
@richtja
Copy link
Contributor Author

richtja commented Jul 1, 2022

Thanks @clebergnu for your comments, I resoled them with force-push.

@richtja richtja requested a review from clebergnu July 1, 2022 13:24
beraldoleal
beraldoleal reviewed Jul 1, 2022
View reviewed changes
Copy link
Member

@beraldoleal beraldoleal left a comment

Choose a reason for hiding this comment

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

Hi Jan, just one small suggestion. But again, good work, LGTM.

avocado/core/plugin_interfaces.py
@abc.abstractmethod
async def save_requirement_in_cache(runtime_task):
"""Saves the information about requirement in cache before
the runtime_task is run.
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: PEP-257, oneline + empty line + description.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was about to change this, but then noticed all docstrings in that file are non-PEP-257 compliant. If you don't mind, let's defer that to a mass change/check effort.

avocado/core/task/runtime.py
failfast = 'FINISHED FAILFAST'
fail_triage = 'FAILED ON TRIAGE'
fail_start = 'FAILED ON START'
started = 'STARTED'
Copy link
Member

Choose a reason for hiding this comment

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

Usually Enum constants are upper case.

@clebergnu clebergnu mentioned this pull request Jul 1, 2022
Merged
@clebergnu
Copy link
Contributor

Closing in favor of #5421

@clebergnu clebergnu closed this Jul 1, 2022
@richtja richtja mentioned this pull request Jul 11, 2022
Merged
@richtja richtja deleted the requirements_database_v2 branch July 13, 2022 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beraldoleal beraldoleal beraldoleal left review comments

@clebergnu clebergnu Awaiting requested review from clebergnu

Assignees

@richtja richtja

Labels
enhancement
Projects
None yet
Milestone
#98 The Thing
Development

Successfully merging this pull request may close these issues.
3 participants
@richtja @clebergnu @beraldoleal