Honor explicit index name #268
Conversation
There was a problem hiding this comment.
Review
- Foreign Key Constraint Name Testing:
o Old Code: Did not include a test for verifying foreign key constraint names.
o New Code: Includes a test (Foreign_key_with_explicit_constraint_name) to verify the explicit constraint name for a foreign key. - Index Name Testing:
o Old Code: Had a basic test for verifying index names (Index method).
o New Code: Extends the index name testing with an additional test (Index_with_explicit_name) to verify the explicit index name. - Tph Method:
o Both the old and new snippets have an empty placeholder for the Tph method, indicating it is not implemented in the provided snippet.
Improvements Needed - Completing Tph Method:
o Implement the Tph method in both old and new code snippets to complete the test suite. - Consistency and Coverage:
o Ensure that all essential aspects of entity configuration (like Table Per Hierarchy mappings) are adequately tested in the unit test suite. - Documentation and Context:
o Document the purpose and expected behavior of each test method to facilitate understanding and maintenance by other developers.
Summary
The updated code snippet introduces tests for verifying explicit names of foreign key constraints and indexes, enhancing coverage and specificity in the unit test suite. Ensure completeness by implementing the Tph method and maintaining consistency across all test methods.
| [Fact] | ||
| public void Index() | ||
| { | ||
| var entityType = BuildEntityType(b => b.Entity<SampleEntity>().HasIndex(s => s.SomeProperty)); | ||
| Assert.Equal("ix_sample_entity_some_property", entityType.GetIndexes().Single().GetDatabaseName()); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void Index_with_explicit_name() |
There was a problem hiding this comment.
Does this test actually fail before this change? Conventions in EF can never override explicit user configuration anyway, so I'm not sure if the change below is actually needed… Can you please confirm?
There was a problem hiding this comment.
Does this test actually fail before this change? Conventions in EF can never override explicit user configuration anyway, so I'm not sure if the change below is actually needed… Can you please confirm?
Yes, this test was failing before the change.
Regarding the second test for a foreign key - this test was not failing and it's there to detect when things may get broken. When I was investigating it, that was working because of a defect in EF. I don't remember all the details, but the code, which is part of EF, was setting an annotation with explicit index name and not firing the event about annotation changed. So, if one day that defect is fixed, explicit index names will be overridden, and the test will show that.
When an index is created and an explicit name is provided, the name should not be rewritten. There is also a unit test to ensure that the explicit name for a foreign key is also honored.