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

Simplify the Result class implementation #1845

Merged
merged 1 commit into from
Feb 22, 2023
Merged

Simplify the Result class implementation #1845

merged 1 commit into from
Feb 22, 2023

Conversation

happz
Copy link
Collaborator

@happz happz commented Feb 10, 2023 •
edited
Loading

  • drop ResultData, it's no longer needed and Result can handle all the work;
  • add guest key as proposed in [1];
  • add schema for validation of (custom) results.yaml;
  • execute step saves results.yaml as a list of results rather than a mapping, to allow one test being executed multiple times. See [2].

[1] #1614 (comment) [2] #1614

@happz happz force-pushed the results-refactored-drop-data branch 3 times, most recently from 3a57468 to 33a2266 Compare February 13, 2023 14:44
@happz happz added step | execute Stuff related to the execute step area | multihost Issues related to the multihost testing support status | blocking other work An important pull request, blocking other pull requests or issues labels Feb 15, 2023
@happz
Copy link
Collaborator Author

happz commented Feb 20, 2023 •
edited
Loading

@psss consider this one for 1.22, please, it'd push the multi-host support a bit further again

@happz happz force-pushed the results-refactored-drop-data branch from 33a2266 to 8c61c85 Compare February 20, 2023 14:21
@psss psss self-assigned this Feb 20, 2023
psss
psss requested changes Feb 20, 2023
View reviewed changes
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 for simplifying this. Looks good in general. Added a few thoughts.

tmt/schemas/results.yaml Outdated Show resolved Hide resolved
tmt/steps/execute/__init__.py Outdated Show resolved Hide resolved
tests/execute/basic/test.sh Outdated Show resolved Hide resolved
tmt/result.py Show resolved Hide resolved
tmt/result.py Show resolved Hide resolved
@thrix thrix added this to the 1.22 milestone Feb 20, 2023
thrix
thrix reviewed Feb 20, 2023
View reviewed changes
tmt/result.py Show resolved Hide resolved
thrix
thrix previously requested changes Feb 20, 2023
View reviewed changes
Copy link
Collaborator

@thrix thrix left a comment

Choose a reason for hiding this comment

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

I believe we can make this into 1.22, looks almost done, seems only minor comments.
Looks cleaner to me.

@happz happz force-pushed the results-refactored-drop-data branch from 8c61c85 to 7b113c7 Compare February 20, 2023 20:22
@psss psss requested a review from thrix February 21, 2023 08:55
@psss
Copy link
Collaborator

psss commented Feb 21, 2023

Please, also rebase on the latest main to fix the missing yq error appearing in tests.

@happz happz force-pushed the results-refactored-drop-data branch from 2c2e9e1 to 7362bc0 Compare February 21, 2023 10:14
@psss psss force-pushed the results-refactored-drop-data branch from 7362bc0 to c9c704c Compare February 21, 2023 16:44
@psss psss changed the title Simplify Result implementation Simplify the Result class implementation Feb 21, 2023
psss
psss approved these changes Feb 21, 2023
View reviewed changes
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 for addressing all comments. Looks good now. Squashed and rebased, now everything should be green!

@happz happz mentioned this pull request Feb 21, 2023
Closed
@happz
Copy link
Collaborator Author

happz commented Feb 22, 2023

/packit retest-failed

@happz
Copy link
Collaborator Author

happz commented Feb 22, 2023 •
edited
Loading

/packit test

@happz
Copy link
Collaborator Author

happz commented Feb 22, 2023

/packit build

@happz happz force-pushed the results-refactored-drop-data branch from c9c704c to 3e2b696 Compare February 22, 2023 12:48
ZelenyMartin
ZelenyMartin reviewed Feb 22, 2023
View reviewed changes
tmt/result.py Outdated Show resolved Hide resolved
@happz @psss
Simplify the Result class implementation
a5d0ed9 
* drop `ResultData`, it's no longer needed and `Result` can handle all
  the work;
* add `guest` key as proposed in [1];
* add schema for validation of (custom) `results.yaml` (the schema is
  not applied yet);
* `execute` step saves `results.yaml` as a list of results rather than a
  mapping, to make allow one test being executed multiple times. See [2].

[1] #1614 (comment)
[2] #1614
@psss psss force-pushed the results-refactored-drop-data branch from c2458f1 to a5d0ed9 Compare February 22, 2023 18:50
@psss psss merged commit a5d0ed9 into main Feb 22, 2023
@psss psss deleted the results-refactored-drop-data branch February 22, 2023 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZelenyMartin ZelenyMartin ZelenyMartin left review comments

@janhavlin janhavlin janhavlin left review comments

@psss psss psss approved these changes

@FrNecas FrNecas Awaiting requested review from FrNecas

@lukaszachy lukaszachy Awaiting requested review from lukaszachy lukaszachy is a code owner

@thrix thrix Awaiting requested review from thrix thrix is a code owner

Assignees

@psss psss

Labels
area | multihost Issues related to the multihost testing support status | blocking other work An important pull request, blocking other pull requests or issues step | execute Stuff related to the execute step
Projects
None yet
Milestone
1.22
Development

Successfully merging this pull request may close these issues.
5 participants
@happz @psss @thrix @ZelenyMartin @janhavlin