Add Shesha.Testing NuGet package for reusable integration test infrastructure#4520
Open
Add Shesha.Testing NuGet package for reusable integration test infrastructure#4520
Conversation
Fix two bugs in SheshaNHibernateInterceptor: null previousState in OnFlushDirty causing NRE during soft-delete detection, and null EntityChangeEventHelper during bootstrap when property injection hasn't run yet. Extract reusable test infrastructure from Shesha.Tests into a new Shesha.Testing NuGet package (ShaIntegratedTestBase, SheshaNhTestBase, database fixtures, ConfigureForTesting helper). This eliminates the need for every Shesha application to copy ~500 lines of test plumbing that was previously locked in the unpublished Shesha.Tests project. Update Shesha.Tests to consume Shesha.Testing, reducing its test module from 142 to 65 lines and removing all duplicated infrastructure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds a new Shesha.Testing project (test bases, fixtures, helpers, README, packaging) and fixes null-reference issues in SheshaNHibernateInterceptor by making EntityChangeEventHelper usage null-safe and guarding previousState in soft-delete detection. Changes
Sequence Diagram(s)sequenceDiagram
participant Module as "Abp Module"
participant Helper as "SheshaTestModuleHelper"
participant IIoc as "IIocManager"
participant Config as "IConfiguration / appsettings.Test.json"
participant DBFixture as "IDatabaseFixture (optional)"
Module->>Helper: ConfigureForTesting(iocManager, configFile)
Helper->>IIoc: Register TestWebHostEnvironment / mocks
Helper->>Config: Load appsettings.Test.json
Helper->>DBFixture: Resolve or read DB settings
Helper->>IIoc: Configure NHibernate / replace services (migrator, repo, email, UoW provider)
Helper->>IIoc: Ensure ApplicationPartManager and identity registrations
Helper-->>Module: Testing environment configured
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
shesha-core/src/Shesha.NHibernate/NHibernate/Interceptors/SheshaNHibernateInterceptor.cs (1)
31-44: 🧹 Nitpick | 🔵 TrivialGood defensive initialization for
EntityChangeEventHelper.The constructor default to
NullEntityChangeEventHelper.Instancecombined with null-conditional access throughout is a solid belt-and-suspenders approach for the early-bootstrap scenario.One minor note: if you want the property initializer to also serve as documentation of the default, you could move it to the property declaration itself:
-public IEntityChangeEventHelper EntityChangeEventHelper { get; set; } +public IEntityChangeEventHelper EntityChangeEventHelper { get; set; } = NullEntityChangeEventHelper.Instance;This makes the default visible at the declaration site and protects against future constructors that forget to set it.
shesha-core/src/Shesha.Testing/ShaIntegratedTestBase.cs (1)
35-57:⚠️ Potential issue | 🟡 Minor
AbpSessionmay be null wheninitializeAbp = false.When the constructor is called with
initializeAbp: false,AbpSession(non-nullable) remains uninitialized untilInitializeAbp()is explicitly called. Accessing it before that point would cause an NRE. Consider making it nullable or documenting the contract clearly.This appears to be pre-existing behavior, but since this is now a shared package, the risk surface increases.
shesha-core/src/Shesha.Testing/Fixtures/LocalSqlServerFixture.cs (1)
20-27:⚠️ Potential issue | 🟡 MinorAdd
appsettings.Test.jsonto test projects usingLocalSqlServerFixture.The fixture reads from
appsettings.Test.jsonusingAddJsonFile()which requires the file by default (optional: false). Test projects must include this file in their output directory or initialization will fail at runtime. The two consuming projects in the repository already have the file properly configured withCopyToOutputDirectory=Always.When the commented-out
LocalDbPersistenceTestsinShesha.Testsis enabled, ensureappsettings.Test.jsonexists there as well.
🤖 Fix all issues with AI agents
In `@shesha-core/src/Shesha.Testing/Fixtures/PostgreSqlFixture.cs`:
- Line 37: The connection string in PostgreSqlFixture.cs is hardcoded to
"127.0.0.1" which can fail in some Docker/CI setups; update the ConnectionString
assignment to use the testcontainer's hostname (use _postgresContainer.Hostname)
instead of the literal IP while keeping the port from
_postgresContainer.GetMappedPublicPort(5432), Database/Username/Password values
remain unchanged so that the _postgresContainer instance provides the correct
host for bridge/remote Docker scenarios.
In `@shesha-core/src/Shesha.Testing/Fixtures/SqlServerFixture.cs`:
- Around line 23-27: The test container image is hardcoded to a private ACR
image which blocks external contributors; make the image configurable and
default to the public Microsoft image. Change the MsSqlBuilder call that
constructs _mssqlContainer to read an environment variable or configuration key
(e.g., MSSQL_TEST_IMAGE) and if unset use
"mcr.microsoft.com/mssql/server:2019-latest" (or similar official tag), leaving
Password and WithExposedPort(1433) unchanged; also update any test setup docs to
mention the MSSQL_TEST_IMAGE override and that private ACR images require
credentials.
In `@shesha-core/src/Shesha.Testing/Shesha.Testing.csproj`:
- Line 33: The PackageReference for Microsoft.SourceLink.GitHub in
Shesha.Testing.csproj is a build-time tool and must not be exposed to consumers;
update the PackageReference element for Microsoft.SourceLink.GitHub to mark it
as a private asset (e.g., add PrivateAssets="All") so the package does not flow
transitively to consumers while keeping it available at build time.
- Around line 2-12: The project file enables packaging via
GeneratePackageOnBuild but lacks essential NuGet metadata; add a <Version>
element (e.g., the correct semantic version for this package) and a
<Description> element inside the same PropertyGroup so the produced NuGet
package has a proper version and description, and add
<IsTestProject>false</IsTestProject> to prevent the SDK from treating this
packable library as a test project; update the PropertyGroup that contains
TargetFramework/PackageId/GeneratePackageOnBuild in Shesha.Testing.csproj to
include these elements.
In `@shesha-core/src/Shesha.Testing/Shesha.Testing.xml`:
- Around line 69-75: The XML docs contain a typo: change "overrided" to
"overridden" in both occurrences (the summary for the member(s) including
M:Shesha.Testing.ShaIntegratedTestBase`1.PostInitialize and the other similar
member), and then decide whether this file is hand-maintained or
auto-generated—if it's produced by <GenerateDocumentationFile> treat it as
generated (remove it from the repo and add the generated file pattern to
.gitignore) otherwise commit the corrected XML text; ensure both summaries are
updated consistently.
In `@shesha-core/src/Shesha.Testing/SheshaNhTestBase.cs`:
- Around line 23-28: The SheshaNhTestBase constructor currently calls
LoginAsHostAdmin() and EntityHelper.RefreshStore(LocalIocManager), which perform
DB I/O at construction time; add an XML documentation comment to the
SheshaNhTestBase(IDatabaseFixture) constructor that clearly states it opens a DB
session and requires the database/fixture to be available before any test runs
(or alternatively note that callers can override/defer LoginAsHostAdmin
invocation if they need to avoid constructor-side DB access), referencing the
constructor name SheshaNhTestBase(IDatabaseFixture) and the LoginAsHostAdmin and
EntityHelper.RefreshStore methods so consumers understand the side effects.
- Around line 59-68: The sync void overload UsingDbSession(int? tenantId,
Action<ISession> action) currently opens a session and invokes action(session)
but never flushes, risking dropped writes; update this method to call
session.Flush() after action(session) and before disposing the session (mirror
the behavior of the other overloads that call Flush/FlushAsync), keeping the
UsingTenantId and OpenSession usage unchanged so the flush happens inside the
using-block that wraps the session.
- Around line 177-191: GetCurrentUserAsync and GetCurrentTenantAsync are passing
the Task-returning SingleAsync calls into the synchronous UsingDbSession
overload which disposes the session before the async query completes; change
both methods to call the asynchronous helper UsingDbSessionAsync and return its
Task, e.g. use UsingDbSessionAsync(session =>
session.Query<User>().SingleAsync(u => u.Id == userId)) and similarly for Tenant
(ensuring the lambda is the async Task-returning delegate), so the session
remains open until the async query finishes.
- Around line 205-211: The NewNhUnitOfWork method is incorrectly
pattern-matching Begin() as an NhUnitOfWork (Begin() returns an
IUnitOfWorkCompleteHandle), so change it to call Resolve<IUnitOfWorkManager>(),
invoke unitOfWorkManager.Begin() (and optionally store the returned
IUnitOfWorkCompleteHandle if you need to dispose it), then read
unitOfWorkManager.Current and cast it to NhUnitOfWork (e.g. use
unitOfWorkManager.Current as NhUnitOfWork) and throw the same exception if the
cast yields null; update NewNhUnitOfWork to use UnitOfWorkManager.Current to
obtain the actual NhUnitOfWork.
In `@shesha-core/src/Shesha.Testing/SheshaTestModuleHelper.cs`:
- Around line 82-84: The code resolves IDatabaseFixture via iocManager.Resolve
and never releases it, risking a Windsor memory leak; replace the direct Resolve
call with a disposable resolve (e.g., use
iocManager.ResolveAsDisposable<IDatabaseFixture>() or wrap the resolved instance
in a using/try/finally and call Release) so the container can clean up
non-singleton fixtures—update the dbFixture allocation in SheshaTestModuleHelper
(the code that currently uses iocManager.Resolve<IDatabaseFixture> and the
dbFixture variable) to obtain a disposable handle and dispose/release it after
use.
- Line 38: The extension-method ConfigureForTesting has an unused receiver
parameter 'this AbpModule module'; either remove the extension receiver if you
don't need extension syntax (make it a normal static method) or keep the
extension form for discoverability but mark the parameter as intentionally
unused by renaming it (e.g., 'this AbpModule _module' or prefixing with '_' )
and add a brief comment above ConfigureForTesting explaining the receiver is
intentionally unused for discoverability to silence warnings and clarify intent.
- Line 65: The current call to
ConfigurationBuilder().AddJsonFile(configFileName) in SheshaTestModuleHelper.cs
will throw if the file is missing; change the call to
AddJsonFile(configFileName, optional: true, reloadOnChange: false) so missing
files don’t crash consumers (or alternatively wrap the build in a try/catch and
surface a clear validation error); update the ConfigurationBuilder usage that
assigns testConfiguration to use the optional/reload flags and, if you choose
optional:true, add validation of required keys after building.
In `@shesha-core/src/Shesha.Testing/TestWebHostEnvironment.cs`:
- Around line 18-19: The WebRootFileProvider and ContentRootFileProvider
properties on TestWebHostEnvironment are initialized with null! which can cause
NREs in tests; change their default initializers to a safe empty provider by
assigning new NullFileProvider() (ensure
Microsoft.Extensions.FileProviders.NullFileProvider is available/imported) so
TestWebHostEnvironment.WebRootFileProvider and
TestWebHostEnvironment.ContentRootFileProvider return a non-null, empty file
provider instead of null.
In `@shesha-core/src/Shesha.Testing/UnitTestHelper.cs`:
- Around line 7-8: The project mixes Moq and NSubstitute; standardize by
migrating MockApiExplorer (the class/method that currently uses Moq) to
NSubstitute so RegisterFakeService can remain unchanged. Replace Moq usage with
NSubstitute.Substitute.For<IApiExplorer>(), remove the Moq using, and configure
any methods/properties on the IApiExplorer substitute to return the same
values/behaviour previously set up with Moq (e.g., set up returns for any
GetApiDescriptions/GetExplorerProperties calls or properties accessed by the
tests). Ensure MockApiExplorer's creation and setups use NSubstitute idioms
(Substitute.For, .Returns, Received checks only if needed) and run tests to
confirm parity.
- Around line 35-42: RegisterFakeService currently unconditionally calls
iocManager.IocContainer.Register which throws if TService is already registered;
update RegisterFakeService(this IIocManager iocManager) to first check
iocManager.IsRegistered<TService>() (or
iocManager.IocContainer.Kernel.HasComponent(typeof(TService)) if IsRegistered
extension not available) and return early when true, otherwise proceed to call
IocContainer.Register with Component.For<TService>().UsingFactoryMethod(() =>
Substitute.For<TService>()).LifestyleSingleton(); ensure you reference the
existing RegisterFakeService method and the IocContainer.Register call when
making the change.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shesha-core/src/Shesha.Testing/Shesha.Testing.csproj`:
- Around line 28-29: This project references two mocking frameworks (Moq and
NSubstitute); remove one to avoid redundant transitive dependencies: decide
which to keep (Moq or NSubstitute), delete the corresponding <PackageReference
Include="Moq" Version="4.20.70" /> or <PackageReference Include="NSubstitute"
Version="5.1.0" /> from Shesha.Testing.csproj, then search the test code for
usages of the removed framework (types/methods from Moq or NSubstitute) and
refactor those tests to the kept framework's API (e.g., replace
Mock<T>/Setup/Verify with Substitute.For<T>/Received or vice versa), and update
any project docs/README and CI restore steps to reflect the single mocking
dependency.
---
Duplicate comments:
In `@shesha-core/src/Shesha.Testing/Shesha.Testing.csproj`:
- Line 36: The PackageReference for Microsoft.SourceLink.GitHub is missing
PrivateAssets="All"; update the PackageReference element for
Include="Microsoft.SourceLink.GitHub" (Version="8.0.0") to add
PrivateAssets="All" so SourceLink is not flowed transitively to consumers, i.e.,
modify the PackageReference with that attribute on the existing
Microsoft.SourceLink.GitHub entry.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@shesha-core/src/Shesha.Testing/UnitTestHelper.cs`:
- Around line 7-8: MockApiExplorer mixes Moq and NSubstitute: replace the
Moq-based mocks in MockApiExplorer with NSubstitute equivalents to match
RegisterFakeService. Remove the "using Moq;" import, change each Moq mock
creation in MockApiExplorer to Substitute.For<T>() for the same interface/type,
update any setup/Verify calls to NSubstitute syntax (Returns/When/Received) and
ensure RegisterFakeService continues to register the substitutes; keep
method/class names unchanged (MockApiExplorer and RegisterFakeService) so wiring
remains identical.
- Around line 50-82: MockApiExplorer currently registers four components
unconditionally which throws on duplicate registration; update MockApiExplorer
to first check whether each service is already registered (use
iocManager.IocContainer.Kernel.HasComponent or the existing IsRegistered-style
helper) before calling IocContainer.Register for
IActionDescriptorCollectionProvider and IApiDescriptionGroupCollectionProvider,
and replace the ISwaggerProvider and ISchemaGenerator registration with calls to
the existing RegisterFakeService<T>() helper (or the same guarded registration)
so they get the duplicate-registration guard and consistent mock setup.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SheshaNHibernateInterceptorthat forced every Sheshaapplication to implement custom interceptor workarounds:
previousStateinOnFlushDirtycausing NRE during soft-deletedetection
EntityChangeEventHelperduring early bootstrap when propertyinjection hasn't run yet
Shesha.TestingNuGet package containing reusable testinfrastructure (
ShaIntegratedTestBase<T>,SheshaNhTestBase<T>,database fixtures for SQL Server/PostgreSQL/local, and a
ConfigureForTesting()helper that encapsulates ~40 lines of commonmodule boilerplate)
Shesha.Teststo consumeShesha.Testing, validating thepackage works and reducing duplicated code
Motivation
Every Shesha application currently copy-pastes ~500 lines of test
infrastructure from the framework's internal
Shesha.Testsprojectbecause it is not published as a NuGet package. This causes:
Test plan
SheshaNHibernateInterceptor.OnFlushDirtyhandles nullpreviousStatewithout throwingSheshaNHibernateInterceptorhandles nullEntityChangeEventHelperduring early bootstrapShesha.Testingpackage builds successfullyShesha.Testspass after consumingShesha.TestingShesha.TestsCloses #4519
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation