Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase coverage of the Updates folder #10648

Merged
merged 24 commits into from
May 14, 2020
Merged

Increase coverage of the Updates folder #10648

merged 24 commits into from
May 14, 2020

Conversation

byorda-glb
Copy link
Contributor

@byorda-glb byorda-glb commented May 13, 2020

Purpose

Increase the tests coverage of the Updates folder

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.

Reviewers

Aaron Tang
Michael Kisrchner

FYIs

Alfredo Pozo


[Test]
[Category("UnitTests")]
public void CheckNewerDailyBuildTest()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this test? This name is misleading as it seems it actually check newer daily builds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I consolidated the tests for the properties and it's now called UpdateManagerConfigurationPropertiesTest

#region UpdateRequest
[Test]
[Category("UnitTests")]
public void UpdateRequest_ConstructorTest()
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not use underscore in test name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the underscore

{
var lookup = new DynamoLookupChildTest();

Assert.IsNull(lookup.LatestProduct);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain or comment here why it would be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment regarding this. It is related to the implementation of GetDynamoInstallLocations in my test class.


[Test]
[Category("UnitTests")]
public void UpdateManagerConfiguration_SaveTest()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please dont use underscore, I think out team are trying to get rid of this coding style recently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the underscore

Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

Looks good but the test names need to reconsidered. If they are property test or function test, I recommend imply that in function name of at least add comments to the test function.

Corrections from the PR review
@QilongTang
Copy link
Contributor

@byorda-glb Thanks, looks good to me!

@QilongTang QilongTang merged commit b6ef6a7 into DynamoDS:master May 14, 2020
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.

2 participants