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
+91 −7
Merged

Merge branch 'develop' into V13 #3447

Show file tree
Hide file tree
Changes from 1 commit
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
12 changes: 10 additions & 2 deletions .github/workflows/build-nightly_v13.yml
View file
Open in desktop
Copy link
Member Author

@rwmcintosh rwmcintosh Mar 16, 2026

Choose a reason for hiding this comment

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

@Yuri05 do we want these changes in V13 also? I removed the schedule for now

Copy link
Member

@Yuri05 Yuri05 Mar 16, 2026

Choose a reason for hiding this comment

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

Don't think it's required.
As long as the workflow build-nightly_v13.yml‎ exists only in develop and in V13 (and starting from develop is prevented by the check there) everything should be fine.

Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ run-name: Version 13.0.${{ github.run_number }}

on:
workflow_dispatch:
schedule:
- cron: '0 2 * * *'

env:
TARGET_FRAMEWORK: net8
Expand All @@ -14,6 +12,16 @@ permissions:
packages: read

jobs:
check-branch:
runs-on: ubuntu-latest
steps:
- name: Validate source branch
run: |
if [ "${{ github.ref_name }}" != "V13" ]; then
echo "::error::Please call the workflow from the V13 branch"
exit 1
fi
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 | 🟠 Major

Branch validation job is not wired as a prerequisite.

The check-branch job validates the branch but no other job depends on it. Without a needs: check-branch dependency on subsequent jobs, this validation runs in parallel and won't actually block the build if the branch check fails.

🔧 Proposed fix to wire the dependency

Add needs: check-branch to the get-latest-commit-timespan job:

   get-latest-commit-timespan:
     runs-on: ubuntu-latest
+    needs: check-branch
     outputs:
       LATEST_COMMIT_TIMESPAN: ${{ steps.get-latest-commit-timespan.outputs.LATEST_COMMIT_TIMESPAN}}

Alternatively, add it to build-nightly as well to ensure both paths are gated:

   build-nightly:
-    needs: get-latest-commit-timespan
+    needs: [check-branch, get-latest-commit-timespan]
     if: needs.get-latest-commit-timespan.outputs.LATEST_COMMIT_TIMESPAN < 86400
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/build-nightly_v13.yml around lines 15 - 23, The branch
validation job check-branch is not set as a prerequisite, so add a needs:
check-branch dependency to the downstream jobs that must be gated (for example
get-latest-commit-timespan and/or build-nightly) so they will fail fast when the
branch check fails; locate the job definitions named get-latest-commit-timespan
and build-nightly in the workflow and insert needs: check-branch under their job
headers to enforce the dependency.


get-latest-commit-timespan:
runs-on: ubuntu-latest
outputs:
Expand Down
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()));
}
}
}