Fixes #1672 Errors are not propagated on failed simulation run #1709
Conversation
| var simulationRunResults = _simModelBatch.RunSimulation(); | ||
|
|
||
| if (!simulationRunResults.Success) | ||
| throw new OSPSuiteException(simulationRunResults.Error); |
There was a problem hiding this comment.
Unsuccessful run must be signaled by exception
| var hasResults = simulationHasResults(_simModelSimulation); | ||
|
|
||
| return new SimulationRunResults(success: hasResults, warnings: WarningsFrom(_simModelSimulation), results: getResults(), | ||
| error: hasResults ? null : Error.SimulationDidNotProduceResults); |
There was a problem hiding this comment.
why don't you create a method (something around the lines of...)
createSimulationResults(string errorFromException = null) {
var hasResults = simulationHasResults(_simModelSimulation);
var warnings = WarningsFrom(_simModelSimulation);
var error = errorFromException ?? (hasResults ? null : Error.SimulationDidNotProduceResults)
if(error==null){
return SimulationRunResults(success: true , warnings, results: getResults())
}
//error
return SimulationRunResults(false, warnings, error)
}
and call that in both instances?
| var id = _ids.First(); | ||
| var simulationBatch = Api.GetSimulationBatchFactory().Create(_simulation, _concurrentRunSimulationBatch.SimulationBatchOptions); | ||
|
|
||
| The.Action(() => simulationBatch.Run(_simulationBatchRunValues.FirstOrDefault(v => v.Id == id))) |
There was a problem hiding this comment.
hum I don't get it. The action was called already in RunConcurrently
So i think this test should check that _results has en entry with success=false and an error
That tests here is a test for the _simulationBatch or sthg. No?
| } | ||
|
|
||
| private bool simulationHasResults(Simulation simModelSimulation) | ||
| { |
There was a problem hiding this comment.
FYI: This is ok to use the short hand syntax (see below)
private bool simulationHasResults(Simulation simModelSimulation) => simModelSimulation.AllValues.Any();
| var warnings = WarningsFrom(_simModelSimulation); | ||
| var error = errorFromException ?? (hasResults ? null : Error.SimulationDidNotProduceResults); | ||
| if (error == null) | ||
| { |
There was a problem hiding this comment.
FYI Also. Not required when a single return
if(error ==null)
return xxx
| [Observation] | ||
| public void should_return_a_result_with_success_set_to_false() | ||
| { | ||
| _results.All(x => x.Succeeded).ShouldBeFalse(); |
There was a problem hiding this comment.
Do we have a test that now verifies that this flag is sometimes true? (e.g. can you set the parameter for one run instead as for all). Then one run should be success and the one where it fails should be false.
…ot cause the whole batch to fail
No description provided.