Conversation
# Conflicts: # .github/workflows/build-nightly_v13.yml # src/PKSim.R/Bootstrap/ApplicationStartup.cs
📝 WalkthroughWalkthroughThis PR adds branch validation to the nightly build workflow, updates GitHub Actions checkout action to v6 in coverage workflow, refactors protocol item enumeration in EventBuildingBlockCreator, and introduces API integration tests verifying successful resolver initialization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
There was a problem hiding this comment.
@Yuri05 do we want these changes in V13 also? I removed the schedule for now
There was a problem hiding this comment.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-nightly_v13.yml:
- Around line 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.
In `@tests/PKSim.R.Tests/APISpecs.cs`:
- Around line 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c884b34d-9fe7-4eca-9322-9811cbb72938
📒 Files selected for processing (4)
.github/workflows/build-nightly_v13.yml.github/workflows/coverage.ymlsrc/PKSim.Core/Services/EventBuildingBlockCreator.cstests/PKSim.R.Tests/APISpecs.cs
| 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 |
There was a problem hiding this comment.
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.
| protected static void ActionShouldNotThrowAn<TException>(Action workToPerform) where TException : Exception | ||
| { | ||
| Exception exceptionFromPerforming = getExceptionFromPerforming(workToPerform); | ||
| (exceptionFromPerforming is TException).ShouldBeFalse(); | ||
| } |
There was a problem hiding this comment.
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.
| 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.
2 participants
merge commit
Summary by CodeRabbit
Chores
Tests