Xcode8 support + #12
Xcode8 support + #12
Conversation
slightair
commented
Sep 25, 2016
slightair
commented
- Remove Quick & Nimble (use XCTest instead)
- Convert test code to Swift3
- Fix broken asynchronous test
- API changes for Swift3
- Update dependencies
| @@ -1,9 +1,6 @@ | |||
| source 'https://github.com/CocoaPods/Specs' | |||
| use_frameworks! | |||
There was a problem hiding this comment.
Is this necessary after removing Quick and Nimble?
| logStore.clearAll() | ||
| } | ||
|
|
||
| func assertLogStoreLogCount(pattern: String, output: PUROutput, expectedCount: Int, line: Int = #line) { |
There was a problem hiding this comment.
In my opinion, this utility's name is verbose.
I suppose Swift 3 style naming
assertLogCount(for pattern: String, to output: PUROutput, shouldBe expectedCount: Int, on line: Int = #line)
| let logger = PURLogger(configuration: loggerConfiguration) | ||
| let testLogStorage = loggerConfiguration.logStorage | ||
|
|
||
| XCTAssertEqual(testLogStorage.description, "") |
There was a problem hiding this comment.
You should use String(describing: obj) instead of calling description directly
|
|
||
| expectation(forNotification: Notification.Name.PURBufferedOutputDidTryWriteChunk.rawValue, object: nil, handler: nil) | ||
| // scheduled after 2sec | ||
| waitForExpectations(timeout: 3.0, handler: nil) |
There was a problem hiding this comment.
why these timeouts are different between its above comments?
There was a problem hiding this comment.
BufferedOutput schedules retry after 2s, 4s, 8s.
So test wait +1 seconds for every retries.
| XCTAssertEqual(testLogStorage.description, "[filter.testXXX|aaa:123][filter.testXXX|bbb:456,ccc:789][filter.testXXX|eee:not filtered]") | ||
|
|
||
| logger.logStore().clearAll() | ||
| logger.shutdown() |
There was a problem hiding this comment.
These seems to be execute on every test cases.
Would you execute them on tearDown.
There was a problem hiding this comment.
I confused test methods behaviour. I fixed it. bcbff49
Respond to #12 review comments
| } | ||
| return self; | ||
| } | ||
|
|
||
| - (void)dealloc { | ||
| self.databaseReadWriteCompletionQueue = nil; |
There was a problem hiding this comment.
[nit] This is unnecessary.
| @@ -54,10 +51,15 @@ - (instancetype)initWithDatabasePath:(NSString *)databasePath | |||
| self = [super init]; | |||
| if (self) { | |||
| _databasePath = databasePath; | |||
| _databaseReadWriteCompletionQueue = dispatch_queue_create("PureeLogStoreReadWriteCompletion", NULL); | |||
There was a problem hiding this comment.
Why did you change the completion queue to the subthread one?
If you change, you need to add some lock-logics to PURBufferedOutput class.
I think using the main-thread makes it simpler.
There was a problem hiding this comment.
You introduce async DB readwrite #6 for avoid UI blocking.
I think better that is some BufferedOutput processes running on the background too.
Do you think that DB read write completion block should running on the main-thread?
There was a problem hiding this comment.
Yes.
DB R/W process is too heavy, but compared to that, BufferedOutput's internal processes are not. Rather than that, using the main-thread makes it much more simpler.
Anyway, currently -(void)flush is called from multiple thread and may sometimes cause NSRangeException. We should decide it as soon as possible.
