Mock api#9
Conversation
ellenspertus
left a comment
ellenspertus
left a comment
There was a problem hiding this comment.
I'd be happy to meet and discuss.
| private void populateChoiceBox() { | ||
| sortChoiceBox.setItems(FXCollections.observableArrayList( | ||
| WordRecord.SortOrder.values())); | ||
| sortChoiceBox.setItems(FXCollections.observableArrayList(WordRecord.SortOrder.values())); |
There was a problem hiding this comment.
The changes to this file are all due to formatting. You should keep this type of change out of PRs. The best way to do so it to look at your PR on GitHub before submitting it.
| * Create sample data to display on the list in JavaFX. | ||
| * | ||
| * @author Ellen Spertus | ||
| * @author Makie Maekawa |
There was a problem hiding this comment.
Excellent! I didn't even ask for people to add class-level javadoc. I should have.
| protected static final int FREQ_YEAR = 2012; | ||
| private static ApiClient client; // set in fillSampleData() | ||
|
|
||
| @VisibleForTesting |
There was a problem hiding this comment.
Reordering the code made it harder to review.
| getFrequencyByYear(wordApi, word, FREQ_YEAR), | ||
| definition.get("text").toString()); | ||
| @VisibleForTesting | ||
| public static String getWord(WordsApi WordsApi) { |
There was a problem hiding this comment.
You should never have a variable name with the same name as a type. I'm surprised that Java allows this. Also, argument names should be lowerCamelCase.
| getFrequencyByYear(wordApi, word, FREQ_YEAR), | ||
| definition.get("text").toString()); | ||
| @VisibleForTesting | ||
| public static String getWord(WordsApi WordsApi) { |
There was a problem hiding this comment.
You don't need this method. Instead, call a method in WordOfTheDay.
| assertEquals(count, getFrequencyByYear(mockWordApi, word, year)); | ||
| } | ||
|
|
||
| private static WordOfTheDay makeWordOfTheDay(String word, List<Object> defin) { |
There was a problem hiding this comment.
It might be clear to make the second argument a String and for this method to call makeMapDefin. That makes things easier for this method's caller and improves abstraction.
| getWordOfTheDay(mockWordsApi).getDefinitions().get(0)); | ||
| } | ||
|
|
||
| @SuppressWarnings("static-access") |
There was a problem hiding this comment.
Rather than suppressing warnings, you should deal with the underlying issue, which I'm happy to discuss.
| @SuppressWarnings("static-access") | ||
| @Test | ||
| void addWordOfTheDay_Equal_correctValue() { | ||
| SampleData sd = mock(SampleData.class); |
There was a problem hiding this comment.
There is no need to mock the class.
| @Test | ||
| void addWordOfTheDay_Equal_correctValue() { | ||
| SampleData sd = mock(SampleData.class); | ||
| when(sd.getWordOfTheDay(mockWordsApi).getDefinitions()).thenReturn(DIFINITION_MAP); |
There was a problem hiding this comment.
You should not mock within the test.
| sd.getWordOfTheDay(mockWordsApi).getWord(), FREQ_YEAR), | ||
| definitionAsMap.get("text").toString()); | ||
|
|
||
| assertTrue(MOCK_WORDRECORD.getWord().equals(testWordRecord.getWord()) |
There was a problem hiding this comment.
You should have a series of assertEquals() statements. Those would give better error messages than a single assertTrue().
2 participants
Mocking Lab
Would you please point out if I misunderstand the instructions?