Skip to content

Conversation

@Bin-QA
Copy link
Contributor

@Bin-QA Bin-QA commented May 29, 2020

use 'trap' command hijack exit command instead of function
'exit' which perpare for 'set -e'

Signed-off-by: Wu, BinX binx.wu@intel.com

@Bin-QA Bin-QA requested review from marc-hb and xiulipan as code owners May 29, 2020 05:37
@aiChaoSONG
Copy link

@Bin-QA Once we use this version of exit, we need to clean up old exit calls in test case, right?

@Bin-QA
Copy link
Contributor Author

Bin-QA commented May 29, 2020

@aiChaoSONG current without more old exit

@aiChaoSONG
Copy link

@Bin-QA , Yes, you are right. when use trap, the func_hijack_exit will be automatically called. Now got the idea

;;
esac

trap '' EXIT
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I missed all the processing done on the exit code, sorry. I don't think you don't have access to the exit code in trap EXIT, so I don't think this can work. How did you test it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you are right, I already test it without this line, it can be worked without recursive call the trap function

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Bin-QA I don't understand your last comment, I don't understand how this can work and I would like to know with what failure and non-zero exit you tested this change because I really don't understand how this can report failures. Try this:

trap 'func_exit_handler' EXIT

func_exit_handler()
{
    printf '$1 = |%s|\n' "$1"
    local exit_status=${1:-0}
    printf '(wrong) exit status  = |%s|\n' "${exit_status}"
}
exit 5

This prints:

$1 = ||
(wrong) exit status  = |0|

Cc: @xiulipan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for miss your point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marc-hb let's track it at gitlab

use 'trap' command hijack exit command instead of function
'exit' which perpare for 'set -e'

Signed-off-by: Wu, BinX <binx.wu@intel.com>
Copy link
Contributor

@xiulipan xiulipan left a comment

Choose a reason for hiding this comment

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

LGTM, let's track the test status to see if this will bring any regression.

@xiulipan xiulipan merged commit 874ca3b into thesofproject:master Jun 5, 2020
@marc-hb
Copy link
Collaborator

marc-hb commented Jun 8, 2020

Fixed in PR #249 / commit fd5a73e

So this #229 was never tested, was it? Testing test code requires making it (manually) FAIL. Test code that has never been red is not tested.

@Bin-QA
Copy link
Contributor Author

Bin-QA commented Jun 9, 2020

Fixed in PR #249 / commit fd5a73e

So this #229 was never tested, was it? Testing test code requires making it (manually) FAIL. Test code that has never been red is not tested.

No, it tested, but it is not display the hide issue from CI system, just confirm the change without effect current logic

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 9, 2020

I still don't understand sorry. Which failure and non-zero exit_status was #229 tested with?

@marc-hb
Copy link
Collaborator

marc-hb commented Jun 10, 2020

There are still a number of builtin exit calls. Before #229, they didn't run the exit handler. Now they all do. Is that not a problem?

@Bin-QA
Copy link
Contributor Author

Bin-QA commented Jun 11, 2020

builtin exit

In case test verify and simple step, it is not effect in current script,
so I push this PR,
but the case result confuse me for the exit_code and I miss understand your point
it is the reason why have PR #249

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants