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

Improve step success logic for sub steps #639

Open
jan-vcapgemini opened this issue Sep 20, 2024 · 0 comments
Open

Improve step success logic for sub steps #639

jan-vcapgemini opened this issue Sep 20, 2024 · 0 comments
Labels
update related to updating software or the entire ide

Comments

@jan-vcapgemini
Copy link
Contributor

jan-vcapgemini commented Sep 20, 2024

In PR #638 several issues were noted, this issue should address them.

Summary:

We could even assert on the step structure here via this.context.getCurrentStep().
In StepImpl we could expose getChildren() for testing.
Just a thought since here we do not assert the status of aggregated steps nor the overall success or failure.
Also by thinking this further and just by reading the code I am now confident that there is an error and this will not work:

if (firstResult.isValid()) {
supressStepSuccess = firstCandidate.isSuppressStepSuccess();
step.success();
return ProcessResult.SUCCESS;
}
}
for (Commandlet cmd : this.commandletManager.getCommandlets()) {
if (cmd != firstCandidate) {
ValidationResult result = applyAndRun(arguments.copy(), cmd);
if (result.isValid()) {
supressStepSuccess = cmd.isSuppressStepSuccess();
step.success();
return ProcessResult.SUCCESS;
}

First of all the overall status should not be success but error and we should get exit code != 0 in such case.
My assumption is even that we will fail with an assertion error if we succeed the root step if it has child steps that are not successful but I have not verified this. At least my expectation is that a step should only be successful if all its child steps are successful.
This is all not your fault and your PR and changes are great. It is just that this review reveals new related problems that I now get aware of. IMHO we should also fix that together. But if you want you can also create a new bug issue and merge this first...

Originally posted by @hohwille in #638 (comment)

Hints

Make sure that if a parent step has child steps which failed, the overall parent step will fail too.
The parent step should only succeed if all child steps were successful.
Make success() return a boolean instead of void.
Replace the return of ProcessResult.SUCCESS with a conditional on the boolean result of success()
We need to create a new test project for this case e.g. install some tools successfully, fail one in between and install more tools successfully and assert the exit code of the overall result.

@jan-vcapgemini jan-vcapgemini added the update related to updating software or the entire ide label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
update related to updating software or the entire ide
Projects
Status: Refinement
Development

No branches or pull requests

1 participant