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

Move abort detection into test invocation #3040

Merged
merged 3 commits into from
Aug 3, 2024

Conversation

happz
Copy link
Collaborator

@happz happz commented Jun 24, 2024

Similar to reboot detection, hiding the details from individual execution plugins.

Pull Request Checklist

  • implement the feature

@happz happz added code | style Code style changes not affecting functionality code | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. labels Jun 24, 2024
@happz happz added this to the 1.35 milestone Jun 24, 2024
Copy link
Collaborator

@therazix therazix left a comment

Choose a reason for hiding this comment

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

LGTM. Just one question. Shouldn't the check_abort_file method be removed from the ExecutePlugin class as it is no longer used?

@happz happz force-pushed the execute-move-stuff-to-invocation-part-two-1 branch from f5263f8 to e0d050c Compare June 28, 2024 13:11
@happz
Copy link
Collaborator Author

happz commented Jun 28, 2024

@therazix absolutely, removed in e0d050c

Copy link
Collaborator

@martinhoyer martinhoyer left a comment

Choose a reason for hiding this comment

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

Nice, I just wonder how it worked before, when there was:

if (abort or self.data.exit_first and
    result.result not in (ResultOutcome.PASS, ResultOutcome.INFO)):

and now we're iterating for result in invocation.results. Was that a bug?

@happz
Copy link
Collaborator Author

happz commented Jul 30, 2024

Nice, I just wonder how it worked before, when there was:

if (abort or self.data.exit_first and
    result.result not in (ResultOutcome.PASS, ResultOutcome.INFO)):

and now we're iterating for result in invocation.results. Was that a bug?

I believe it was a bug, because it was checking result.result, and result was whatever the loop saw as the last item, most likely invocation.results[-1]:

                if abort:
                    for result in invocation.results:
                        # In case of aborted all results in list will be aborted
                        result.note = 'aborted'

...

                for result in invocation.results:
                    logger.verbose(
                        f"{_format_duration(result)} {result.show()} [{progress}]",
                        shift=shift)

...

                if (abort or self.data.exit_first and
                        result.result not in (ResultOutcome.PASS, ResultOutcome.INFO)):

So only the last result was considered, which does not seem right.

@thrix thrix added status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. ci | full test Pull request is ready for the full test execution labels Jul 31, 2024
@thrix
Copy link
Collaborator

thrix commented Jul 31, 2024

/packit test

Similar to reboot detection, hiding the details from individual
execution plugins.
@happz happz force-pushed the execute-move-stuff-to-invocation-part-two-1 branch from e0d050c to 24ed894 Compare August 3, 2024 07:23
@happz happz merged commit 63b1035 into main Aug 3, 2024
20 checks passed
@happz happz deleted the execute-move-stuff-to-invocation-part-two-1 branch August 3, 2024 10:43
kkaarreell pushed a commit that referenced this pull request Aug 6, 2024
Similar to reboot detection, hiding the details from individual
execution plugins.
kkaarreell pushed a commit that referenced this pull request Aug 6, 2024
Similar to reboot detection, hiding the details from individual
execution plugins.
The-Mule pushed a commit to The-Mule/tmt that referenced this pull request Oct 14, 2024
Similar to reboot detection, hiding the details from individual
execution plugins.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci | full test Pull request is ready for the full test execution code | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. code | style Code style changes not affecting functionality status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants