Skip to content

Unit Tests for SuggestionsTableView#19018

Merged
twstokes merged 6 commits intowordpress-mobile:trunkfrom
salimbraksa:task/suggestions-tests
Jul 8, 2022
Merged

Unit Tests for SuggestionsTableView#19018
twstokes merged 6 commits intowordpress-mobile:trunkfrom
salimbraksa:task/suggestions-tests

Conversation

@salimbraksa
Copy link
Contributor

@salimbraksa salimbraksa commented Jul 8, 2022

Issue

Part of #18979

Description

This adds Unit Tests to SuggestionsTableView most critical methods, which are:

  • showSuggestions(forWord:)
  • searchResults(searchQuery:suggestions:suggestionType)
  • moveProminentSuggestionsToTop(searchResults:prominentSuggestionsIds:)
  • prominentSuggestions(fromPostAuthorId:commentAuthorId:defaultAccountId:)

Test Instructions

  • Make sure WordPress target is selected
  • Show the Test Navigator
  • Filter by SuggestionsTableViewTests
  • Run Tests
  • Verify that all tests are green ✅

Regression Notes

  1. Potential unintended areas of impact
    n/a

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    n/a

  3. What automated tests I added (or what prevented me from doing so)
    n/a

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@salimbraksa salimbraksa changed the title Tests for SuggestionsTableView [WIP] Tests for SuggestionsTableView Jul 8, 2022
expectedResult3.insert(element, at: 0)
element = expectedResult3.remove(at: 14)
expectedResult3.insert(element, at: 1)
let result3 = self.suggestionsTableView.moveProminentSuggestionsToTop(searchResults: input, prominentSuggestionsIds: [10, 15])
Copy link
Contributor Author

@salimbraksa salimbraksa Jul 8, 2022

Choose a reason for hiding this comment

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

@twstokes Is it okey to construct these expectedResults like this?

Another alternative is to have those expectations as JSON files, then load them the same way we are loading user-suggestions.json.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's OK, but it may be even clearer to split up those cases into their own tests. I think tests are a place where we can be more verbose and not as focused on being DRY.

I've personally been a fan of the "given / when / then" format.

@salimbraksa salimbraksa force-pushed the task/suggestions-tests branch from ee95dc5 to f73c719 Compare July 8, 2022 20:36
@salimbraksa salimbraksa requested a review from twstokes July 8, 2022 21:47
@salimbraksa salimbraksa force-pushed the task/suggestions-tests branch from 122827a to 6a62524 Compare July 8, 2022 21:48
self.suggestionsTableView.suggestions = suggestions
}

override func tearDownWithError() throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

It'll be fine to wipe this if we don't need it. 👍

@salimbraksa salimbraksa marked this pull request as ready for review July 8, 2022 22:22
@salimbraksa salimbraksa changed the title [WIP] Tests for SuggestionsTableView Unit Tests for SuggestionsTableView Jul 8, 2022
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

Copy link
Contributor

@twstokes twstokes 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 @salimbraksa! Thanks for adding these. 🎉

@twstokes twstokes added this to the 20.3 milestone Jul 8, 2022
@twstokes twstokes merged commit 2388b0f into wordpress-mobile:trunk Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants