Skip to content

refactor: remove mocks from CliParametersAnalyzerTest#854

Merged
lionel-nj merged 7 commits intomasterfrom
remove-mocks
May 4, 2021
Merged

refactor: remove mocks from CliParametersAnalyzerTest#854
lionel-nj merged 7 commits intomasterfrom
remove-mocks

Conversation

@lionel-nj
Copy link
Copy Markdown
Contributor

@lionel-nj lionel-nj commented Apr 15, 2021

closes #591

Summary:

This PR provides support to:

  • remove mocks from CliParametersAnalyzerTest
  • fix randomly failing tests on CI

Expected behavior:

Tests should pass on CI.

Please make sure these boxes are checked before submitting your pull request - thanks!

@lionel-nj lionel-nj self-assigned this Apr 15, 2021
@lionel-nj lionel-nj requested a review from barbeau April 15, 2021 17:05
lionel-nj added 2 commits April 26, 2021 09:11
# Conflicts:
#	main/src/test/java/org/mobilitydata/gtfsvalidator/cli/CliParametersAnalyzerTest.java
@lionel-nj
Copy link
Copy Markdown
Contributor Author

Thanks for review @aababilov, PTAL.

@lionel-nj lionel-nj requested a review from aababilov April 26, 2021 13:40
// verifyNoMoreInteractions(mockArguments, mockHandler);
// }
@Test
public void noUrlNoInput_isNotValid() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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();
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads-up @aababilov!

Copy link
Copy Markdown
Collaborator

@aababilov aababilov left a comment

Choose a reason for hiding this comment

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

Please also remove mockito from main/build.gradle.

lionel-nj added 4 commits May 3, 2021 14:47
- remove dependency on mockito
- split test
- remove test for short names
- rename static helper method for clarity
@lionel-nj
Copy link
Copy Markdown
Contributor Author

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 ArgumentsTest already checks arguments initialization whether we're using short or long names.

@lionel-nj lionel-nj requested a review from aababilov May 3, 2021 18:54
Copy link
Copy Markdown
Collaborator

@aababilov aababilov left a comment

Choose a reason for hiding this comment

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

Thanks!

@lionel-nj lionel-nj merged commit 4824a4c into master May 4, 2021
@lionel-nj lionel-nj deleted the remove-mocks branch May 4, 2021 13:56
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.

CLI tests fail randomly on CI

3 participants