Skip to content

Merge branch 'develop' into V13 #3447

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.

Sign up for GitHub

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

Merged
rwmcintosh merged 2 commits into from
Mar 16, 2026
Merged

Merge branch 'develop' into V13 #3447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f887036
Merge branch 'develop' into V13
rwmcintosh Mar 16, 2026
e2f2491
pr feedback
rwmcintosh Mar 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/coverage.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jobs:
runs-on: windows-latest
steps:
- name: Checkout code
uses: actions/checkout@v4
uses: actions/checkout@v6
with:
submodules: 'true'

Expand Down
9 changes: 3 additions & 6 deletions src/PKSim.Core/Services/EventBuildingBlockCreator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,10 @@ private void addProtocol(CompoundProperties compoundProperties)

eventGroup.SourceCriteria.Add(new MatchTagCondition(CoreConstants.Tags.EVENTS));

var schemaItems = _schemaItemsMapper.MapFrom(protocol).ToList();
var width = schemaItems.Count.ToString().Length;

schemaItems.Each((schemaItem, index) =>
_schemaItemsMapper.MapFrom(protocol).Each((schemaItem, index) =>
{
var number = (index + 1).ToString($"D{width}");
var applicationName = $"{CoreConstants.APPLICATION_NAME_TEMPLATE}{number}";
//+1 to start at 1 for the nomenclature
var applicationName = $"{CoreConstants.APPLICATION_NAME_TEMPLATE}{index + 1}";
addApplication(eventGroup, schemaItem, applicationName, compoundProperties, protocol);
});

Expand Down
87 changes: 87 additions & 0 deletions tests/PKSim.R.Tests/APISpecs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
using System;
using Castle.MicroKernel.ComponentActivator;
using NUnit.Framework;
using OSPSuite.BDDHelper;
using OSPSuite.BDDHelper.Extensions;
using OSPSuite.CLI.Core.RunOptions;
using PKSim.CLI.Core.RunOptions;

namespace PKSim.R
{
[IntegrationTests]
[Category("R")]
public class APISpecs : StaticContextSpecification
{
public override void GlobalContext()
{
base.GlobalContext();
Api.InitializeOnce();

Environment.CurrentDirectory = AppDomain.CurrentDomain.BaseDirectory;
}

private static Exception getExceptionFromPerforming(Action work)
{
try
{
work();
return null;
}
catch (Exception ex)
{
return ex;
}
}

protected static void ActionShouldNotThrowAn<TException>(Action workToPerform) where TException : Exception
{
Exception exceptionFromPerforming = getExceptionFromPerforming(workToPerform);
(exceptionFromPerforming is TException).ShouldBeFalse();
}
Comment on lines +36 to +40
Copy link

@coderabbitai coderabbitai bot Mar 16, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Assertion helper silently passes when a different exception type is thrown.

If workToPerform throws an exception that is not of type TException, the assertion still passes because exceptionFromPerforming is TException evaluates to false. This masks actual failures.

For resolution tests, if a dependency throws ArgumentNullException instead of ComponentActivatorException, the test would pass incorrectly.

🔧 Proposed fix to fail on any exception or specifically check resolution succeeded

Option 1 - Fail on any exception:

 protected static void ActionShouldNotThrowAn<TException>(Action workToPerform) where TException : Exception
 {
    Exception exceptionFromPerforming = getExceptionFromPerforming(workToPerform);
-   (exceptionFromPerforming is TException).ShouldBeFalse();
+   if (exceptionFromPerforming != null)
+   {
+      (exceptionFromPerforming is TException).ShouldBeFalse(
+         $"Expected no {typeof(TException).Name} but got: {exceptionFromPerforming}");
+   }
 }

Option 2 - If the intent is purely to test resolution, consider calling Api.ResolveTask<T>() directly instead of invoking the full runner execution.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
protected static void ActionShouldNotThrowAn<TException>(Action workToPerform) where TException : Exception
{
Exception exceptionFromPerforming = getExceptionFromPerforming(workToPerform);
(exceptionFromPerforming is TException).ShouldBeFalse();
}
protected static void ActionShouldNotThrowAn<TException>(Action workToPerform) where TException : Exception
{
Exception exceptionFromPerforming = getExceptionFromPerforming(workToPerform);
if (exceptionFromPerforming != null)
{
(exceptionFromPerforming is TException).ShouldBeFalse(
$"Expected no {typeof(TException).Name} but got: {exceptionFromPerforming}");
}
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/PKSim.R.Tests/APISpecs.cs` around lines 36 - 40, The helper
ActionShouldNotThrowAn<TException> incorrectly treats any non-TException as
passing; update it so that it fails when any exception is thrown by asserting
exceptionFromPerforming is null (or using the test framework's
Assert.Null/ShouldBeNull) instead of comparing types, e.g. in
ActionShouldNotThrowAn<TException> replace the (exceptionFromPerforming is
TException).ShouldBeFalse() check with a check that exceptionFromPerforming is
null (or explicitly fail with the exception message if not null), or if the
intent is to verify resolution succeeded call Api.ResolveTask<T>() directly from
your test rather than using the runner; use getExceptionFromPerforming to locate
the produced exception and include its message in the failure.

}

public class When_resolving_tasks_after_initialization : APISpecs
{
[Observation]
public void can_get_individual_factory()
{
Api.GetIndividualFactory().ShouldNotBeNull();
}

[Observation]
public void can_get_population_factory()
{
Api.GetPopulationFactory().ShouldNotBeNull();
}

[Observation]
public void can_resolve_snapshot_runner()
{
ActionShouldNotThrowAn<ComponentActivatorException>(() => Api.RunSnapshot(new SnapshotRunOptions()));
}

[Observation]
public void can_resolve_export_runner()
{
ActionShouldNotThrowAn<ComponentActivatorException>(() => Api.RunExport(new ExportRunOptions()));
}

[Observation]
public void can_resolve_json_runner()
{
ActionShouldNotThrowAn<ComponentActivatorException>(() => Api.RunJson(new JsonRunOptions()));
}

[Observation]
public void can_resolve_qualification_runner()
{
ActionShouldNotThrowAn<ComponentActivatorException>(() => Api.RunQualification(new QualificationRunOptions()));
}

[Observation]
public void can_resolve_simulation_export_runner()
{
ActionShouldNotThrowAn<ComponentActivatorException>(() => Api.RunSimulationExport(new ExportRunOptions()));
}
}
}