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

Context (former Relevancy) implementation #83

Merged
merged 3 commits into from Nov 13, 2020
Merged

Context (former Relevancy) implementation #83

merged 3 commits into from Nov 13, 2020

Conversation

lukaszachy
Copy link
Collaborator

Let's start with examples for rules

@thrix
Copy link
Collaborator

thrix commented Oct 6, 2020

Some examples from other fields, like versions from poetry (but it is related to packaging):

https://python-poetry.org/docs/versions/

Not applicable fully, but something could apply also to our use cases

@thrix
Copy link
Collaborator

thrix commented Oct 6, 2020

library for parsing semver is here, but not sure if helps us :) https://github.com/python-poetry/semver

@lgtm-com
Copy link

lgtm-com bot commented Oct 27, 2020

This pull request introduces 3 alerts when merging 34b828b into 6570aa5 - view on LGTM.com

new alerts:

  • 3 for Unused local variable

@coveralls
Copy link

coveralls commented Oct 27, 2020

Pull Request Test Coverage Report for Build 411

  • 197 of 197 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 386: 0.0%
Covered Lines: 797
Relevant Lines: 797

💛 - Coveralls

@thrix
Copy link
Collaborator

thrix commented Oct 27, 2020

@lukaszachy nice code man 👍

@lukaszachy
Copy link
Collaborator Author

@thrix thank you ;) and also for semver hint - I'll try to use it.
@psss I guess this could be used already, SUT() and SUT.matches() should be all you need to use from TMT.
Better name proposals welcome ;)

My plan for next days is to have 100% coverage and do docstrings + documentation.

After that I'll squash commits and resolve WIP

@psss
Copy link
Collaborator

psss commented Nov 9, 2020

Thanks for implementing this, @lukaszachy. I haven't reviewed the whole code but gave it a bit of testing and it works as expected. I was thinking a bit about the name and would like to propose Context instead of SUT. From the definition:

the circumstances that form the setting for an event, statement, or idea, and in terms of which it can be fully understood and assessed.

As we want to use not only environment dimensions but also things like trigger with values such as pull request, errata or compose to define in which context a test/plan is relevant, term context sounds general enough.

And second naming proposal: We were not sure about the rule attribute. I propose adjust which nicely describes what the rules are intended for: To adjust metadata based on rules. A simple example for illustration:

summary: Just a simple smoke test
test: tmt --help
enabled: true
adjust:
    enable: false
    when: distro = centos

What do you think?

@psss
Copy link
Collaborator

psss commented Nov 9, 2020

It seems that the following two unit tests fail:

TestExample.test_simple_conditions
TestExample.test_minor_comparisson_mode

@lukaszachy
Copy link
Collaborator Author

Worked on my system, I'll look at those flaky tests and will rename class

@psss
Copy link
Collaborator

psss commented Nov 10, 2020

Thanks. A couple of Python 2 compatiblity issues fixed in b703ce0. Feel free to squash them as well.

@lukaszachy
Copy link
Collaborator Author

Squashed previous commits, fixed tests. If tests pass then I'll rename the class and will ask for review.

@lukaszachy lukaszachy marked this pull request as ready for review November 11, 2020 10:34
@lukaszachy lukaszachy changed the title WIP - relevancy Context (former Relevancy) implementation Nov 11, 2020
@lukaszachy lukaszachy requested a review from psss November 11, 2020 10:35
@lukaszachy
Copy link
Collaborator Author

Todo

  • nice docstring for (at least) public classes / methods
  • chapter about Context to user-facing documentation (scope: motivation, rule grammar, intended usage)

Mostly style adjustments plus fixed several typos.
Slightly updated the Context example section.
Added and enabled a new 'Context' page in docs.
Copy link
Collaborator

@psss psss left a comment

Choose a reason for hiding this comment

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

Thanks much for putting this together, @lukaszachy. That must have been a great patience and endurance exercise! Added a few comment and questions. Going to push a bunch of style adjustments in a separate commit as agreed.

fmf/context.py Outdated Show resolved Hide resolved
fmf/context.py Outdated Show resolved Hide resolved
fmf/context.py Outdated Show resolved Hide resolved
fmf/context.py Outdated Show resolved Hide resolved
fmf/context.py Outdated Show resolved Hide resolved
fmf/context.py Show resolved Hide resolved
fmf/context.py Outdated Show resolved Hide resolved
tests/unit/test_context.py Outdated Show resolved Hide resolved
@lukaszachy lukaszachy requested a review from psss November 12, 2020 15:37
@lukaszachy
Copy link
Collaborator Author

lukaszachy commented Nov 12, 2020

I expect a lot of changes for documentation, so it is in it's own PR: #86

From code/working PoV this PR is ready.

Copy link
Collaborator

@psss psss left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for the adjustments. Just two typos.

fmf/context.py Outdated Show resolved Hide resolved
tests/unit/test_context.py Outdated Show resolved Hide resolved
@psss psss self-assigned this Nov 13, 2020
@psss psss added the enhancement New feature or request label Nov 13, 2020
Operator changes:
defined -> is defined
!defined -> is not defined
&& -> and
|| -> or
@psss
Copy link
Collaborator

psss commented Nov 13, 2020

Thanks, merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants