Skip to content

Andrew Added testing command and cases for testing localization#46

Open
AlbertOpus2507 wants to merge 5 commits into
mainfrom
test/localization
Open

Andrew Added testing command and cases for testing localization#46
AlbertOpus2507 wants to merge 5 commits into
mainfrom
test/localization

Conversation

@AlbertOpus2507
Copy link
Copy Markdown

Added test cases for deep copy and also easier testing in root directory

@lhmcgann
Copy link
Copy Markdown
Owner

Edit this PR to merge back onto release-v2.0 rather than main branch

Comment thread EStimLibrary.sln Outdated
Comment thread src/EStimLibrary/Core/SpatialModel/LocalizationData.cs Outdated
public record LocalizationData(IEnumerable<int> AreasFullyContaining,
IEnumerable<int> AreasPartiallyContaining)
{
// Modify the properties using ToList() to create shallow copies during initialization
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What did doing this fix?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By doing this the merge method already working with the deep copied version of the input rather than the actual input itself, just another layer of protection.

using System.Collections.Generic;
using System.Linq;

namespace EStimLibrary.UnitTests.Core.SpatialModel
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style thing: 2 newlines between using and namespace declarations, then also put a ; at the end of the namespace declaration line so you can get rid of the outermost {} and unindent everything by one

/// ensure immutability (encapsulation).
/// </summary>
[Fact]
public void LocalizationData_Constructor()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove the class name prefix from each test method name. Just start with the name of the method you're testing (e.g., Constructor or Merge). See the "Our Coding Practices" notebook page > As You Go... section > item 4 for more test method naming convention description

public void LocalizationData_Constructor()
{
// Arrange
var areasFullyContaining = new List<int> { 1, 2, 3 };
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to test different values for each method; try to think of edge cases! You can give your test methods the [Theory] flag instead of the [Fact] flag so you can use [InlineData(...)] or [MemberData(...)] flags to pass in multiple test values without needing to copy the body of the test method over and over again

Copy link
Copy Markdown

@bacchuongdaungo bacchuongdaungo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks ok to me

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants