chore(graphql): Add model property names test #1434
Merged
Conversation
This file contains 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
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a series of unit tests across multiple files to validate the property names of various GraphQL object types against their corresponding domain entities. The tests ensure that the GraphQL representations accurately reflect the structure of the domain models by comparing property names, while excluding specific properties from the validation process. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (10)
tests/Digdir.Domain.Dialogporten.GraphQl.Unit.Tests/ObjectTypes/TransmissionTests.cs (2)
8-37: Add XML documentation to explain the test's purpose and property exclusions.The test would benefit from documentation explaining why certain properties are excluded from the comparison.
Apply this diff:
+ /// <summary> + /// Validates that all properties from the domain DialogTransmission entity are represented in the GraphQL Transmission type. + /// </summary> + /// <remarks> + /// Properties are excluded from validation when they: + /// - Represent navigation properties (e.g., RelatedTransmission, Dialog) + /// - Are internal identifiers (e.g., DialogId, TypeId) + /// - Represent collections (e.g., Activities, RelatedTransmissions) + /// </remarks> [Fact] public void Transmission_Object_Type_Should_Match_Property_Names_On_DialogTransmission()
22-37: Consider enhancing the property validation.The current implementation has some limitations:
- Case-insensitive comparison might mask naming inconsistencies
- Only checks for missing properties in one direction
- No validation that GraphQL doesn't have extra properties
Consider this enhanced implementation:
- var domainTransmissionProperties = typeof(DialogTransmission) + var domainProperties = typeof(DialogTransmission) .GetProperties() .Select(p => p.Name) .Where(name => !ignoreList.Contains(name)) .ToList(); - var transmissionProperties = typeof(Transmission) + var graphqlProperties = typeof(Transmission) .GetProperties() .Select(p => p.Name) .ToList(); - var missingProperties = domainTransmissionProperties.Except(transmissionProperties, StringComparer.OrdinalIgnoreCase).ToList(); + // Check for case-sensitive matches + var missingInGraphQL = domainProperties.Except(graphqlProperties).ToList(); + var extraInGraphQL = graphqlProperties.Except(domainProperties).ToList(); + + // Check for case mismatches + var caseMismatches = domainProperties + .Where(dp => graphqlProperties.Any(gp => + string.Equals(dp, gp, StringComparison.OrdinalIgnoreCase) && + !string.Equals(dp, gp, StringComparison.Ordinal))) + .ToList(); // Assert - Assert.True(missingProperties.Count == 0, $"Properties missing in graphql transmission: {string.Join(", ", missingProperties)}"); + Assert.Empty(missingInGraphQL, $"Properties missing in GraphQL: {string.Join(", ", missingInGraphQL)}"); + Assert.Empty(extraInGraphQL, $"Extra properties in GraphQL: {string.Join(", ", extraInGraphQL)}"); + Assert.Empty(caseMismatches, $"Properties with case mismatches: {string.Join(", ", caseMismatches)}");tests/Digdir.Domain.Dialogporten.GraphQl.Unit.Tests/ObjectTypes/GuiActionTests.cs (2)
12-20: Document the rationale for ignored properties.Consider adding XML comments or a summary explaining why these specific properties are excluded from the comparison. This helps future maintainers understand the test's assumptions.
+ // Properties excluded from comparison: + // - CreatedAt/UpdatedAt: Internal tracking fields not exposed in GraphQL + // - PriorityId/HttpMethodId: Internal FK fields represented differently in GraphQL + // - DialogId/Dialog: Navigation properties handled separately in GraphQL var ignoreList = new List<string>
22-40: Consider enhancing test maintainability and coverage.A few suggestions to improve this test:
- Extract the property comparison logic into a reusable helper method
- Add a reverse check to ensure no extra properties exist in GraphQL type
- Enhance the assertion message with both expected and actual property sets
Here's a suggested refactor:
+ private static void AssertPropertyNamesMatch<TDomain, TGraphQL>( + IEnumerable<string> ignoreList = null) + { + var domainProps = typeof(TDomain).GetProperties() + .Select(p => p.Name) + .Where(name => ignoreList?.Contains(name) != true) + .ToList(); + + var graphqlProps = typeof(TGraphQL).GetProperties() + .Select(p => p.Name) + .ToList(); + + var missingInGraphQL = domainProps + .Except(graphqlProps, StringComparer.OrdinalIgnoreCase) + .ToList(); + + var extraInGraphQL = graphqlProps + .Except(domainProps, StringComparer.OrdinalIgnoreCase) + .ToList(); + + Assert.True( + missingInGraphQL.Count == 0 && extraInGraphQL.Count == 0, + $""" + Property name mismatch between {typeof(TDomain).Name} and {typeof(TGraphQL).Name}: + Missing in GraphQL: {string.Join(", ", missingInGraphQL)} + Extra in GraphQL: {string.Join(", ", extraInGraphQL)} + """); + }tests/Digdir.Domain.Dialogporten.GraphQl.Unit.Tests/ObjectTypes/DialogEntityTests.cs (4)
14-22: Document the rationale for ignored properties.Consider adding comments explaining why each property is excluded from the comparison. This will help future maintainers understand the test's design decisions.
var ignoreList = new List<string> { + // Excluded as it's an internal tracking property nameof(DialogEntity.Deleted), + // Excluded as it's an internal timestamp nameof(DialogEntity.DeletedAt), + // Excluded as it's replaced by Status in GraphQL nameof(DialogEntity.StatusId), + // Excluded as it's handled differently in GraphQL nameof(DialogEntity.SearchTags), nameof(DialogEntity.SeenLog), nameof(DialogEntity.DialogEndUserContext) };
24-39: Consider refactoring for better maintainability and clarity.The property comparison logic could be improved for better maintainability and more precise error reporting.
Consider these improvements:
+private static readonly IReadOnlySet<string> IgnoredProperties = new HashSet<string> +{ + nameof(DialogEntity.Deleted), + // ... other properties +}; + +private static List<string> GetPropertyNames(Type type, ISet<string> ignoreList = null) => + type.GetProperties() + .Select(p => p.Name) + .Where(name => ignoreList?.Contains(name) != true) + .ToList(); + [Fact] public void Dialog_Object_Type_Should_Match_Property_Names_On_DialogEntity() { - // Arrange - var ignoreList = new List<string> - { - // ... current list - }; - - var dialogProperties = typeof(DialogEntity) - .GetProperties() - .Select(p => p.Name) - .Where(name => !ignoreList.Contains(name)) - .ToList(); - - var domainDialogProperties = typeof(Dialog) - .GetProperties() - .Select(p => p.Name) - .ToList(); + var dialogProperties = GetPropertyNames(typeof(DialogEntity), IgnoredProperties); + var domainDialogProperties = GetPropertyNames(typeof(Dialog)); var missingProperties = dialogProperties.Except(domainDialogProperties, StringComparer.OrdinalIgnoreCase).ToList(); - // Assert - Assert.True(missingProperties.Count == 0, $"Properties missing in graphql dialog: {string.Join(", ", missingProperties)}"); + Assert.Empty(missingProperties); }
41-59: Consider standardizing the test implementation.This test could benefit from the same improvements suggested for the first test method. Additionally, consider making the comparison case-sensitive to catch potential casing mismatches.
[Fact] public void DialogStatus_Object_Type_Should_Match_Property_Names_On_DialogStatusValues() { - // Arrange - var dialogStatusValues = typeof(DialogStatus) - .GetProperties() - .Select(p => p.Name) - .ToList(); - - var domainDialogStatusValues = typeof(DialogStatusValues) - .GetProperties() - .Select(p => p.Name) - .ToList(); + var dialogStatusValues = GetPropertyNames(typeof(DialogStatus)); + var domainDialogStatusValues = GetPropertyNames(typeof(DialogStatusValues)); - var missingProperties = domainDialogStatusValues.Except(dialogStatusValues, StringComparer.OrdinalIgnoreCase).ToList(); + // Use case-sensitive comparison to catch casing mismatches + var missingProperties = domainDialogStatusValues.Except(dialogStatusValues).ToList(); - // Assert - Assert.True(missingProperties.Count == 0, $"Properties missing in graphql dialog status: {string.Join(", ", missingProperties)}"); + Assert.Empty(missingProperties); }
8-60: Consider enhancing the test architecture.The tests could benefit from a more structured approach to improve maintainability and readability:
- Consider implementing a custom assertion for property comparison:
public static class PropertyAssert { public static void PropertiesMatch<T1, T2>( ISet<string> ignoredProperties = null, bool caseSensitive = true) { var comparer = caseSensitive ? StringComparer.Ordinal : StringComparer.OrdinalIgnoreCase; // ... implementation } }
- Consider using the test data builder pattern if the test setup grows more complex.
This would make the tests more maintainable and provide better error messages.
tests/Digdir.Domain.Dialogporten.GraphQl.Unit.Tests/ObjectTypes/AttachmentTests.cs (2)
1-2: Consider more explicit type aliases.The current aliases could be more descriptive to better distinguish between GraphQL and domain types.
-using Attachment = Digdir.Domain.Dialogporten.GraphQL.EndUser.DialogById.Attachment; -using DomainAttachmentUrl = Digdir.Domain.Dialogporten.Domain.Attachments.AttachmentUrl; +using GraphQlAttachment = Digdir.Domain.Dialogporten.GraphQL.EndUser.DialogById.Attachment; +using GraphQlAttachmentUrl = Digdir.Domain.Dialogporten.GraphQL.EndUser.DialogById.AttachmentUrl; +using DomainAttachment = Digdir.Domain.Dialogporten.Domain.Attachments.Attachment; +using DomainAttachmentUrl = Digdir.Domain.Dialogporten.Domain.Attachments.AttachmentUrl;
6-70: Consider extracting common test logic to a base class.Both test methods share similar property comparison logic. Consider creating a base test class to reduce code duplication and standardize the testing approach across all GraphQL type tests.
Example base class:
public abstract class GraphQlTypeTestBase { protected void ValidatePropertyNames<TGraphQl, TDomain>(IEnumerable<string> ignoreList) { var graphQlProperties = typeof(TGraphQl) .GetProperties() .Select(p => p.Name) .ToList(); var domainProperties = typeof(TDomain) .GetProperties() .Select(p => p.Name) .Where(name => !ignoreList.Contains(name)) .ToList(); var missingProperties = domainProperties .Except(graphQlProperties, StringComparer.OrdinalIgnoreCase) .ToList(); var extraProperties = graphQlProperties .Except(domainProperties, StringComparer.OrdinalIgnoreCase) .ToList(); Assert.True(missingProperties.Count == 0, $"Properties missing in GraphQL type: {string.Join(", ", missingProperties)}"); Assert.True(extraProperties.Count == 0, $"Extra properties in GraphQL type: {string.Join(", ", extraProperties)}"); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
tests/Digdir.Domain.Dialogporten.GraphQl.Unit.Tests/ObjectTypes/AttachmentTests.cs(1 hunks)tests/Digdir.Domain.Dialogporten.GraphQl.Unit.Tests/ObjectTypes/DialogEntityTests.cs(1 hunks)tests/Digdir.Domain.Dialogporten.GraphQl.Unit.Tests/ObjectTypes/GuiActionTests.cs(1 hunks)tests/Digdir.Domain.Dialogporten.GraphQl.Unit.Tests/ObjectTypes/TransmissionTests.cs(1 hunks)
🔇 Additional comments (3)
tests/Digdir.Domain.Dialogporten.GraphQl.Unit.Tests/ObjectTypes/TransmissionTests.cs (1)
1-5: LGTM! Clean and minimal imports.
The imports are focused and relevant to the test requirements.
tests/Digdir.Domain.Dialogporten.GraphQl.Unit.Tests/ObjectTypes/DialogEntityTests.cs (1)
1-6: LGTM! Well-organized imports with clear alias usage.
The imports are properly organized and use clear aliases to avoid naming conflicts between domain and GraphQL types.
tests/Digdir.Domain.Dialogporten.GraphQl.Unit.Tests/ObjectTypes/AttachmentTests.cs (1)
38-69: Document property exclusions and consider design implications.
The large number of excluded properties might indicate a potential mismatch between the domain and GraphQL models.
Let's verify if these exclusions are consistent with other similar tests:
Consider:
- Adding XML documentation explaining why each property is excluded
- Evaluating if the GraphQL type could better align with the domain model
- Adding bidirectional validation like in the previous test
+ /// <summary>
+ /// Validates that all domain model properties are present in the GraphQL type, excluding:
+ /// - CreatedAt, UpdatedAt: Audit fields not exposed in GraphQL
+ /// - ConsumerTypeId, AttachmentId: Internal reference fields
+ /// - Attachment: Navigation property handled separately
+ /// </summary>
[Fact]
public void AttachmentUrl_Object_Type_Should_Match_Property_Names_On_DialogAttachmentUrl()
{
// Similar improvements as suggested for the first test method
}✅ Verification successful
The search results show a consistent pattern across multiple test files where certain properties are excluded from GraphQL types. These exclusions follow clear patterns:
- Audit fields (
CreatedAt,UpdatedAt,Deleted,DeletedAt) - Internal IDs and references (
ConsumerTypeId,AttachmentId,PriorityId,HttpMethodId,StatusId) - Navigation properties (
Attachment,Dialog,RelatedTransmission, etc.)
Property exclusions follow established patterns and architectural decisions
The large number of excluded properties in the AttachmentUrl test aligns with the project's consistent approach across different GraphQL types, where:
- Audit fields are systematically excluded from the GraphQL layer
- Internal IDs and references are hidden from the public API
- Navigation properties are handled separately in the GraphQL schema
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar property exclusion patterns in other test files
rg -A 5 "ignoreList = new List<string>" tests/
Length of output: 3758
Comment on lines
+39
to
+57
| [Fact] | ||
| public void TransmissionType_Object_Type_Should_Match_Property_Names_On_DialogTransmissionTypeValues() | ||
| { | ||
| // Arrange | ||
| var transmissionTypes = typeof(TransmissionType) | ||
| .GetFields() | ||
| .Select(f => f.Name) | ||
| .ToList(); | ||
|
|
||
| var domainTransmissionTypes = typeof(DialogTransmissionType.Values) | ||
| .GetFields() | ||
| .Select(f => f.Name) | ||
| .ToList(); | ||
|
|
||
| var missingProperties = domainTransmissionTypes.Except(transmissionTypes, StringComparer.OrdinalIgnoreCase).ToList(); | ||
|
|
||
| // Assert | ||
| Assert.True(missingProperties.Count == 0, $"Properties missing in graphql TransmissionType: {string.Join(", ", missingProperties)}"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance enum validation and add documentation.
The test would benefit from documentation and more thorough validation.
Apply these improvements:
+ /// <summary>
+ /// Validates that all enum values from the domain DialogTransmissionType are represented in the GraphQL TransmissionType.
+ /// </summary>
[Fact]
public void TransmissionType_Object_Type_Should_Match_Property_Names_On_DialogTransmissionTypeValues()
{
// Arrange
- var transmissionTypes = typeof(TransmissionType)
+ var graphqlEnumValues = typeof(TransmissionType)
.GetFields()
- .Select(f => f.Name)
+ .Where(f => f.IsLiteral && !f.IsSpecialName)
+ .ToDictionary(f => f.Name, f => f.GetRawConstantValue())
.ToList();
- var domainTransmissionTypes = typeof(DialogTransmissionType.Values)
+ var domainEnumValues = typeof(DialogTransmissionType.Values)
.GetFields()
- .Select(f => f.Name)
+ .Where(f => f.IsLiteral && !f.IsSpecialName)
+ .ToDictionary(f => f.Name, f => f.GetRawConstantValue())
.ToList();
- var missingProperties = domainTransmissionTypes.Except(transmissionTypes, StringComparer.OrdinalIgnoreCase).ToList();
+ var missingInGraphQL = domainEnumValues.Keys
+ .Except(graphqlEnumValues.Keys)
+ .ToList();
+
+ var extraInGraphQL = graphqlEnumValues.Keys
+ .Except(domainEnumValues.Keys)
+ .ToList();
+
+ var valueMismatches = domainEnumValues
+ .Where(dv => graphqlEnumValues.ContainsKey(dv.Key) &&
+ !Equals(dv.Value, graphqlEnumValues[dv.Key]))
+ .Select(dv => dv.Key)
+ .ToList();
// Assert
- Assert.True(missingProperties.Count == 0, $"Properties missing in graphql TransmissionType: {string.Join(", ", missingProperties)}");
+ Assert.Empty(missingInGraphQL,
+ $"Enum values missing in GraphQL: {string.Join(", ", missingInGraphQL)}");
+ Assert.Empty(extraInGraphQL,
+ $"Extra enum values in GraphQL: {string.Join(", ", extraInGraphQL)}");
+ Assert.Empty(valueMismatches,
+ $"Enum values with mismatched values: {string.Join(", ", valueMismatches)}");
}This enhancement:
- Adds documentation explaining the test's purpose
- Validates both enum names and their underlying values
- Checks for missing and extra values in both directions
- Provides clearer error messages
📝 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
| [Fact] | |
| public void TransmissionType_Object_Type_Should_Match_Property_Names_On_DialogTransmissionTypeValues() | |
| { | |
| // Arrange | |
| var transmissionTypes = typeof(TransmissionType) | |
| .GetFields() | |
| .Select(f => f.Name) | |
| .ToList(); | |
| var domainTransmissionTypes = typeof(DialogTransmissionType.Values) | |
| .GetFields() | |
| .Select(f => f.Name) | |
| .ToList(); | |
| var missingProperties = domainTransmissionTypes.Except(transmissionTypes, StringComparer.OrdinalIgnoreCase).ToList(); | |
| // Assert | |
| Assert.True(missingProperties.Count == 0, $"Properties missing in graphql TransmissionType: {string.Join(", ", missingProperties)}"); | |
| } | |
| /// <summary> | |
| /// Validates that all enum values from the domain DialogTransmissionType are represented in the GraphQL TransmissionType. | |
| /// </summary> | |
| [Fact] | |
| public void TransmissionType_Object_Type_Should_Match_Property_Names_On_DialogTransmissionTypeValues() | |
| { | |
| // Arrange | |
| var graphqlEnumValues = typeof(TransmissionType) | |
| .GetFields() | |
| .Where(f => f.IsLiteral && !f.IsSpecialName) | |
| .ToDictionary(f => f.Name, f => f.GetRawConstantValue()) | |
| .ToList(); | |
| var domainEnumValues = typeof(DialogTransmissionType.Values) | |
| .GetFields() | |
| .Where(f => f.IsLiteral && !f.IsSpecialName) | |
| .ToDictionary(f => f.Name, f => f.GetRawConstantValue()) | |
| .ToList(); | |
| var missingInGraphQL = domainEnumValues.Keys | |
| .Except(graphqlEnumValues.Keys) | |
| .ToList(); | |
| var extraInGraphQL = graphqlEnumValues.Keys | |
| .Except(domainEnumValues.Keys) | |
| .ToList(); | |
| var valueMismatches = domainEnumValues | |
| .Where(dv => graphqlEnumValues.ContainsKey(dv.Key) && | |
| !Equals(dv.Value, graphqlEnumValues[dv.Key])) | |
| .Select(dv => dv.Key) | |
| .ToList(); | |
| // Assert | |
| Assert.Empty(missingInGraphQL, | |
| $"Enum values missing in GraphQL: {string.Join(", ", missingInGraphQL)}"); | |
| Assert.Empty(extraInGraphQL, | |
| $"Extra enum values in GraphQL: {string.Join(", ", extraInGraphQL)}"); | |
| Assert.Empty(valueMismatches, | |
| $"Enum values with mismatched values: {string.Join(", ", valueMismatches)}"); | |
| } |
Comment on lines
+42
to
+63
| [Fact] | ||
| public void GuiActionPriority_Object_Type_Should_Match_Property_Names_On_DialogGuiActionPriorityValues() | ||
| { | ||
| // Arrange | ||
| var guiActionPriorities = typeof(GuiActionPriority) | ||
| .GetProperties() | ||
| .Select(p => p.Name) | ||
| .ToList(); | ||
|
|
||
| var domainGuiActionPriorities = typeof(DialogGuiActionPriority.Values) | ||
| .GetProperties() | ||
| .Select(p => p.Name) | ||
| .ToList(); | ||
|
|
||
| var missingProperties = domainGuiActionPriorities | ||
| .Except(guiActionPriorities, StringComparer.OrdinalIgnoreCase) | ||
| .ToList(); | ||
|
|
||
| // Assert | ||
| Assert.True(missingProperties.Count == 0, | ||
| $"Properties missing in graphql GuiActionPriority: {string.Join(", ", missingProperties)}"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance enum value validation.
The current test only checks property names but for enums, we should also validate that the actual values match between domain and GraphQL types.
Consider adding value comparison:
[Fact]
public void GuiActionPriority_Object_Type_Should_Match_Property_Names_On_DialogGuiActionPriorityValues()
{
- // Arrange
- var guiActionPriorities = typeof(GuiActionPriority)
- .GetProperties()
- .Select(p => p.Name)
- .ToList();
-
- var domainGuiActionPriorities = typeof(DialogGuiActionPriority.Values)
- .GetProperties()
- .Select(p => p.Name)
- .ToList();
-
- var missingProperties = domainGuiActionPriorities
- .Except(guiActionPriorities, StringComparer.OrdinalIgnoreCase)
- .ToList();
-
- // Assert
- Assert.True(missingProperties.Count == 0,
- $"Properties missing in graphql GuiActionPriority: {string.Join(", ", missingProperties)}");
+ // Check property names
+ AssertPropertyNamesMatch<DialogGuiActionPriority.Values, GuiActionPriority>();
+
+ // Check enum values match
+ var domainValues = typeof(DialogGuiActionPriority.Values)
+ .GetProperties()
+ .ToDictionary(p => p.Name, p => p.GetValue(null));
+
+ var graphqlValues = typeof(GuiActionPriority)
+ .GetProperties()
+ .ToDictionary(p => p.Name, p => p.GetValue(null));
+
+ foreach (var (name, domainValue) in domainValues)
+ {
+ Assert.True(
+ graphqlValues.TryGetValue(name, out var graphqlValue) &&
+ domainValue?.ToString() == graphqlValue?.ToString(),
+ $"Value mismatch for {name}: Domain={domainValue}, GraphQL={graphqlValue}");
+ }
}Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+8
to
+36
| [Fact] | ||
| public void Attachment_Object_Type_Should_Match_Property_Names_On_AttachmentEntity() | ||
| { | ||
| // Arrange | ||
| var ignoreList = new List<string> | ||
| { | ||
| nameof(Domain.Attachments.Attachment.CreatedAt), | ||
| nameof(Domain.Attachments.Attachment.UpdatedAt) | ||
| }; | ||
|
|
||
| var attachmentProperties = typeof(Attachment) | ||
| .GetProperties() | ||
| .Select(p => p.Name) | ||
| .ToList(); | ||
|
|
||
| var domainAttachmentProperties = typeof(Domain.Attachments.Attachment) | ||
| .GetProperties() | ||
| .Select(p => p.Name) | ||
| .Where(name => !ignoreList.Contains(name)) | ||
| .ToList(); | ||
|
|
||
| var missingProperties = domainAttachmentProperties | ||
| .Except(attachmentProperties, StringComparer.OrdinalIgnoreCase) | ||
| .ToList(); | ||
|
|
||
| // Assert | ||
| Assert.True(missingProperties.Count == 0, | ||
| $"Properties missing in graphql Attachment: {string.Join(", ", missingProperties)}"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and documentation.
While the test validates domain properties exist in GraphQL type, consider these improvements:
- Add XML documentation explaining the test's purpose and excluded properties
- Add bidirectional validation to ensure GraphQL type doesn't have extra properties
- Add test cases for inheritance scenarios
+ /// <summary>
+ /// Validates that all domain model properties (except audit fields) are present in the GraphQL type.
+ /// Excluded properties:
+ /// - CreatedAt: Audit field not exposed in GraphQL
+ /// - UpdatedAt: Audit field not exposed in GraphQL
+ /// </summary>
[Fact]
public void Attachment_Object_Type_Should_Match_Property_Names_On_AttachmentEntity()
{
// Arrange
var ignoreList = new List<string>
{
nameof(Domain.Attachments.Attachment.CreatedAt),
nameof(Domain.Attachments.Attachment.UpdatedAt)
};
var attachmentProperties = typeof(Attachment)
.GetProperties()
.Select(p => p.Name)
.ToList();
var domainAttachmentProperties = typeof(Domain.Attachments.Attachment)
.GetProperties()
.Select(p => p.Name)
.Where(name => !ignoreList.Contains(name))
.ToList();
+ // Check domain properties exist in GraphQL type
var missingProperties = domainAttachmentProperties
.Except(attachmentProperties, StringComparer.OrdinalIgnoreCase)
.ToList();
+ // Check no extra properties in GraphQL type
+ var extraProperties = attachmentProperties
+ .Except(domainAttachmentProperties, StringComparer.OrdinalIgnoreCase)
+ .ToList();
// Assert
Assert.True(missingProperties.Count == 0,
$"Properties missing in graphql Attachment: {string.Join(", ", missingProperties)}");
+ Assert.True(extraProperties.Count == 0,
+ $"Extra properties in graphql Attachment: {string.Join(", ", extraProperties)}");
}📝 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
| [Fact] | |
| public void Attachment_Object_Type_Should_Match_Property_Names_On_AttachmentEntity() | |
| { | |
| // Arrange | |
| var ignoreList = new List<string> | |
| { | |
| nameof(Domain.Attachments.Attachment.CreatedAt), | |
| nameof(Domain.Attachments.Attachment.UpdatedAt) | |
| }; | |
| var attachmentProperties = typeof(Attachment) | |
| .GetProperties() | |
| .Select(p => p.Name) | |
| .ToList(); | |
| var domainAttachmentProperties = typeof(Domain.Attachments.Attachment) | |
| .GetProperties() | |
| .Select(p => p.Name) | |
| .Where(name => !ignoreList.Contains(name)) | |
| .ToList(); | |
| var missingProperties = domainAttachmentProperties | |
| .Except(attachmentProperties, StringComparer.OrdinalIgnoreCase) | |
| .ToList(); | |
| // Assert | |
| Assert.True(missingProperties.Count == 0, | |
| $"Properties missing in graphql Attachment: {string.Join(", ", missingProperties)}"); | |
| } | |
| /// <summary> | |
| /// Validates that all domain model properties (except audit fields) are present in the GraphQL type. | |
| /// Excluded properties: | |
| /// - CreatedAt: Audit field not exposed in GraphQL | |
| /// - UpdatedAt: Audit field not exposed in GraphQL | |
| /// </summary> | |
| [Fact] | |
| public void Attachment_Object_Type_Should_Match_Property_Names_On_AttachmentEntity() | |
| { | |
| // Arrange | |
| var ignoreList = new List<string> | |
| { | |
| nameof(Domain.Attachments.Attachment.CreatedAt), | |
| nameof(Domain.Attachments.Attachment.UpdatedAt) | |
| }; | |
| var attachmentProperties = typeof(Attachment) | |
| .GetProperties() | |
| .Select(p => p.Name) | |
| .ToList(); | |
| var domainAttachmentProperties = typeof(Domain.Attachments.Attachment) | |
| .GetProperties() | |
| .Select(p => p.Name) | |
| .Where(name => !ignoreList.Contains(name)) | |
| .ToList(); | |
| // Check domain properties exist in GraphQL type | |
| var missingProperties = domainAttachmentProperties | |
| .Except(attachmentProperties, StringComparer.OrdinalIgnoreCase) | |
| .ToList(); | |
| // Check no extra properties in GraphQL type | |
| var extraProperties = attachmentProperties | |
| .Except(domainAttachmentProperties, StringComparer.OrdinalIgnoreCase) | |
| .ToList(); | |
| // Assert | |
| Assert.True(missingProperties.Count == 0, | |
| $"Properties missing in graphql Attachment: {string.Join(", ", missingProperties)}"); | |
| Assert.True(extraProperties.Count == 0, | |
| $"Extra properties in graphql Attachment: {string.Join(", ", extraProperties)}"); | |
| } |
arealmaas
approved these changes
Nov 11, 2024
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Tests ensuring property/enum values are matching from domain => graphql
Adds test for Dialog, Transmissions, GuiAction, and Attatchments
Summary by CodeRabbit
New Features
Attachment,DialogEntity,GuiAction, andTransmissiontypes to enhance reliability and consistency.Tests