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

Fixes #1464 throw when running the same twice #1468

Merged
merged 3 commits into from
Feb 2, 2022

Conversation

msevestre
Copy link
Member

No description provided.

@msevestre msevestre requested a review from abdelr February 1, 2022 15:18
/// <param name="cancellationToken">Cancellation token to cancel the threads</param>
/// <param name="numberOfCoresToUse">Number of cores to use. Use null to take all cores</param>
/// <returns>Dictionary binding a result for each input data after running the action on it</returns>
Task RunAsync<TData>(
Copy link
Member Author

Choose a reason for hiding this comment

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

@abdelr As discussed. Thanks for the suggestion

{
var id = generateId();
simulationBatchRunValues.Id = id;
Copy link
Member Author

Choose a reason for hiding this comment

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

I am moving the creation of the Id to the object itselft...

return _concurrencyManager.RunAsync(
batches,
//Use {} to specify using the overload without return value
(batch, ct) => { batch.AddNewBatch(); },
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only downside of using the same name... I need to use the {} to that it picks the right overload

@@ -186,19 +189,30 @@ public void should_import_simple_data_from_csv()
}

[Observation]
public void should_return_empty_on_invalid_file_name()
[TestCase("IntegrationSample1.csv", 2)]
[TestCase("sample_non_existent.csv", 2)]
Copy link
Member Author

Choose a reason for hiding this comment

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

oups... this does not belong here!

@abdelr
Copy link
Member

abdelr commented Feb 1, 2022

@msevestre Tests are failing


_simulationBatch1.AddSimulationBatchRunValues(_parValues1);
_simulationBatch2.AddSimulationBatchRunValues(_parValues1);
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding the same object multiple time is not allowed anymore... unique ids

Copy link
Member Author

@msevestre msevestre left a comment

Choose a reason for hiding this comment

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

@abdelr Ready to review

new ConcurrencyManagerResult<TResult>(datum.Id, action(datum, cancellationToken))
);
}
catch (Exception e)
Copy link
Member

Choose a reason for hiding this comment

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

You removed the try/catch. This was here to make sure that if one run fails (running one simulation throws an exception for example) the rest are still recorded. Like this I think you would not be able to return anything at all. Right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep. But here we have no way of knowing which one failed as we have no identifier.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough.

@msevestre
Copy link
Member Author

Tests are running again. This was the wrong file commited.

@msevestre msevestre merged commit 0a1c7d8 into develop Feb 2, 2022
@msevestre msevestre deleted the 1464-throw-when-running-the-same-twice branch February 2, 2022 10:52
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.

None yet

2 participants