refactor: remove mocks from CliParametersAnalyzerTest#854
Conversation
main/src/test/java/org/mobilitydata/gtfsvalidator/cli/CliParametersAnalyzerTest.java
Outdated
Show resolved
Hide resolved
# Conflicts: # main/src/test/java/org/mobilitydata/gtfsvalidator/cli/CliParametersAnalyzerTest.java
|
Thanks for review @aababilov, PTAL. |
| // verifyNoMoreInteractions(mockArguments, mockHandler); | ||
| // } | ||
| @Test | ||
| public void noUrlNoInput_isNotValid() { |
There was a problem hiding this comment.
Again, you have multiple assertThat in each test. You should split tests even more. You see, each scenario is fully independent from the rest. You should also remove the temporary variables.
Test names should describe the scenario so that you can remove the comments completely. A good code is usually understandable without comments. You should be able to make the code self-documenting rather than resorting to a comment. Comments that document the "what" are usually easy to get rid of; it's the "why" comments that are hard to replace. Use a comment when it is infeasible to make your code self-explanatory.
@Test
public void noUrlNoInput_long_isNotValid() {
assertThat(validateArguments(new String[]{
"--output_base", "output value",
"--threads", "4"
})).isFalse();
}
Also, there is no difference between scenarios with (--output_base, --threads) vs (-o, -t) since url and input are not provided anyway. You may have instead:
@Test
public void noUrlNoInput_isNotValid() {
assertThat(validateArguments(new String[]{})).isFalse();
}
aababilov
left a comment
There was a problem hiding this comment.
Please also remove mockito from main/build.gradle.
- remove dependency on mockito - split test - remove test for short names - rename static helper method for clarity
|
Thanks for review @aababilov! I modified the tests following your suggestions. I only kept test using long names since as you mentioned previously: using the short names for the same scenarios is not relevant - given that |
closes #591
Summary:
This PR provides support to:
CliParametersAnalyzerTestExpected behavior:
Tests should pass on CI.
Please make sure these boxes are checked before submitting your pull request - thanks!
gradle testto make sure you didn't break anything[ ] Include screenshot(s) showing how this pull request works and fixes the issue(s)