Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chioma Amanda Mmegwa iOSTestingBootcamp #9

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

arturoDiaz93
Copy link

Created to be able to comment and leave feedback.

Copy link
Author

@arturoDiaz93 arturoDiaz93 left a comment

Choose a reason for hiding this comment

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

Hey Chisma! Amazing work done here! 🚀 🎸

You are on 98.4% of code coverage which it is great. I wasn't able to find bigger issues other than some clean code practices and something more. Check those comments. So, basically it is cool how you learned and implemented TDD on this project. Keep learning more about testing and I would recommend you to learn more about swift code style, SOLID and SoC design principles, memory management (ARC, Capture Lists, Reference Types) and just to finish, I know you could be buried with work and other stuff, but It could have been great if you updated the UI of the app. Congratulations again! Keep learning new stuff :D.

protocol FeedDataManagerProtocol {
func fetch(completion: (Result<[TweetCellViewModel], Error>) -> Void)
func fetch(completion: @escaping (Result<[TweetCellViewModel], Error>) -> Void)
Copy link
Author

Choose a reason for hiding this comment

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

Nice catch!

protocol FeedDataManagerProtocol {
func fetch(completion: (Result<[TweetCellViewModel], Error>) -> Void)
func fetch(completion: @escaping (Result<[TweetCellViewModel], Error>) -> Void)
}

class FeedDataManager: FeedDataManagerProtocol {
Copy link
Author

Choose a reason for hiding this comment

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

You could add the final key for this class.


func fetch(completion: @escaping (Result<[TweetCellViewModel], Error>) -> Void) {
let url = URL(string: "https://gist.githubusercontent.com/ferdelarosa-wz/0c73ab5311c845fb7dfac4b62ab6c652/raw/6a39cffe68d87f1613f222372c62bd4e89ad06fa/tweets.json")
guard let url else { return }
Copy link
Author

Choose a reason for hiding this comment

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

It is great to see that you are avoiding force unwrapping.

completion(.failure(error))
return
}

Copy link
Author

Choose a reason for hiding this comment

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

Extra empty line is not needed.

@@ -27,13 +27,13 @@ class FeedViewModel {
self.dataManager = dataManager
}
func fetchTimeline() {
dataManager.fetch { result in
dataManager.fetch { [weak self] result in
Copy link
Author

Choose a reason for hiding this comment

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

Nice catch!

Comment on lines +92 to +114
private class FakeSession: URLSession {
var data: Data?
var error: Error?

override func dataTask(with request: URLRequest, completionHandler: @escaping (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask {

MockDataTask {
completionHandler(self.data, nil, self.error)
}
}
}

private class MockDataTask: URLSessionDataTask {
private let closure: () -> ()

init(closure: @escaping () -> ()) {
self.closure = closure
}

override func resume() {
closure()
}
}
Copy link
Author

Choose a reason for hiding this comment

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

This clases should go on its own files

Comment on lines +116 to +120
override func tearDown() {
super.tearDown()
sut = nil
session = nil
}
Copy link
Author

Choose a reason for hiding this comment

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

This should go above. after the set up.

@testable import MiniBootcamp

final class TweetModelTests: XCTestCase {
func test_mapToViewModel() {
Copy link
Author

Choose a reason for hiding this comment

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

You could leave and extra line after the class declaration.

Comment on lines +14 to +24
user: User(
followersCount: 20,
name: "test",
screenName: "screenName",
description: "description",
location: "location",
createdAt: "createdAt",
profileBackgroundColor: "backgroundColor",
profileImageURL: "imageUrl",
profileBackgroundImageURL: "backgroundImageUrl"
),
Copy link
Author

Choose a reason for hiding this comment

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

For readability, declare the user property apart and then use it to build the tweet struct.

@testable import MiniBootcamp

final class TweetModelTests: XCTestCase {
func test_mapToViewModel() {
Copy link
Author

Choose a reason for hiding this comment

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

You should be consistent on the naming convention of your test methods. Either use this snake case or the camel case used on the other tests. Not big deal but consistency is great.

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.

2 participants