Skip to content

DatabaseTargetTests - Add coverage for DbConnectionFactory constructor#67

Open
JohnVerheij wants to merge 1 commit intoNLog:masterfrom
JohnVerheij:tests/databasetarget-factory-ctor-coverage
Open

DatabaseTargetTests - Add coverage for DbConnectionFactory constructor#67
JohnVerheij wants to merge 1 commit intoNLog:masterfrom
JohnVerheij:tests/databasetarget-factory-ctor-coverage

Conversation

@JohnVerheij
Copy link
Copy Markdown
Contributor

Adds direct test coverage for the DatabaseTarget(Func<IDbConnection>) constructor introduced in NLog/NLog#5878. The factory constructor was previously covered indirectly through DbTypeEnumSetterTest, DbTypeSetterTest, and DbFactoryConnectionStringTest1/2; this brings parity with the existing tests for the legacy DBProvider constructor.

Tests added:

  • DbConnectionFactoryNullTest - null factory throws ArgumentNullException
  • DbConnectionFactoryReturnsNullTest - factory returning null surfaces NLogRuntimeException
  • DbConnectionFactoryThrowsTest - factory exceptions propagate to the async continuation
  • DbConnectionFactorySimpleWriteTest - end-to-end single write
  • DbConnectionFactoryBatchedWriteTest - end-to-end batched write
  • DbConnectionFactoryKeepConnectionTest - persistent-connection mode
  • DbConnectionFactoryWithParametersWriteTest - end-to-end write with Action<IDbDataParameter> parameter setter

All 222 tests pass on net8.0. No production code changes.

@snakefoot
Copy link
Copy Markdown
Contributor

Thank you for the contribution. Since there already multiple tests that verifies that _dbConnectionFactory works when called correctly, then I think only these tests are interesting:

  • DbConnectionFactoryNullTest - Might not compile if enabling nullable for the unit-test-project.
  • DbConnectionFactoryReturnsNullTest - Might not compile if enabling nullable for the unit-test-project.
  • DbConnectionFactoryThrowsTest - This is a good test.
  • DbConnectionFactoryWithParametersWriteTest - Like this test as it combines both dbConnection-delegate and dbParameter-delegate.

I think these tests are already covered by existing test:

  • DbConnectionFactoryKeepConnectionTest
  • DbConnectionFactoryBatchedWriteTest
  • DbConnectionFactorySimpleWriteTest

@JohnVerheij
Copy link
Copy Markdown
Contributor Author

The 7 tests cover the DatabaseTarget(Func<IDbConnection>) constructor added in NLog/NLog#5878. Before this PR the user-provided factory variant has no direct tests; the existing suite covers the connection-string constructor only.

The 4 you want to keep cover defensive cases (null factory, factory-returns-null, factory-throws) and parameters-write. The 3 you want to drop (Simple, Batched, KeepConnection) give the factory constructor parity with the write-mode coverage the existing tests provide for the connection-string constructor.

Architecturally both constructors set _dbConnectionFactory and the single OpenConnection method invokes that field for every write (single and batched, KeepConnection on or off). There is no downstream branching by constructor. Under that invariant the contested 3 are redundant; they serve as regression coverage at the public API in case a future refactor introduces divergence. Will drop if you'd rather rely on the architecture invariant alone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants