Skip to content

Conversation

@Lyssia-Seiden
Copy link
Contributor

No description provided.

Copy link
Contributor

@Awesomeplayer165 Awesomeplayer165 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@kevinclark kevinclark left a comment

Choose a reason for hiding this comment

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

Unclear this is useful yet and expect it's in progress. Some notes though.

}

@Test
private void initTest() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer test names that say what is expected. These are never called, so you should be excessively verbose since it's what's going to communicate intent when it fails. F.e, "robotShouldNotCombustWhenFedFrechfries()" would be a totally reasonable unit test method name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also note you don't event need test in the name - you've already communicated it's a test with the annotation

Copy link
Contributor

Choose a reason for hiding this comment

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

All that being said, what's your goal with this method? When does it fail? What does it tell you when it does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the goal here is just to make sure Robot initializes without throwing an error, mainly as a simple template for unit tests as we (hopefully) add more throughout the season

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I'd make it explicit with a assertDoesNotThrow. Communicates your intent and makes clear you didn't forget the assert along the way.

"java.test.defaultConfig": "WPIlibUnitTests",
"java.compile.nullAnalysis.mode": "automatic"
"java.compile.nullAnalysis.mode": "automatic",
"wpilib.skipTests": true
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this set to true on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, to avoid excessive delays during deploys from running unit tests, and instead my plan was to run these tests in ci

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.

4 participants