Refactor FXIOS-9270 Add debug logging for intermittent ContentBlockerTest failure #20597
Conversation
Generated by 馃毇 Danger Swift against c39b2b5 |
There was a problem hiding this comment.
A test failed but not the one we wanted
| ContentBlocker.shared.compileListsNotInStore { | ||
| let finishDate = Date() | ||
| print(finishDate.timeIntervalSince(startDate)) |
There was a problem hiding this comment.
can you put some pointers like "MT Test Time - (finishDate.timeIntervalSince(startDate))" to make it easier to find it, also this won't be pushed to production so we can play around with adding nslog as well
There was a problem hiding this comment.
Will add better debug logging 馃槄
Also, since the test failure is intermittent, would it be worth merging to production, that way we can passively gather these metrics with all other PR's instead of just re-running this one until it occurs again?
There was a problem hiding this comment.
We don't merge print statements to prod but if you are interested in learning more about performance testing then you can run something similar in that manner
Tagging @isabelrios for pointers here on how to measure performance of a method
There was a problem hiding this comment.
Sorry fo the late reply here. Yeah, we try not to use print statments in our tests.
In the Performance tests we use measure and the metrics its provides. Not sure if that would serve your needs... but just in case
nbhasin2
added
the
Do Not Merge 鉀旓笍
MattLichtenstein
changed the title
| ContentBlocker.shared.compileListsNotInStore { | ||
| let finishDate = Date() | ||
| NSLog("MT Test Time - \(finishDate.timeIntervalSince(startDate))") |
There was a problem hiding this comment.
Going to tag @isabelrios and @clarmso in case you do something specific for performance integration testing
There was a problem hiding this comment.
@MattLichtenstein do we need a change to remove the logging?
As I understand this is temporary we can add a ticket to remove it once we have the data needed for this
@nbhasin2 Yep, created FXIOS-9351: Remove debug logging from ContentBlockerTests:testCompileListsNotInStore_callsCompletionHandlerSuccessfully. Shall we remove the "Do no merge" label? |
You can remove it, although I would ask @isabelrios for a quick review before merging |
MattLichtenstein
removed
the
Do Not Merge 鉀旓笍
MattLichtenstein
deleted the
ml/FXIOS-9270_testCompileListsNotInStore_callsCompletionHandlerSuccessfully-intermittent-failure
branch
馃摐 Tickets
Jira ticket
Github issue
馃挕 Description
testCompileListsNotInStore_callsCompletionHandlerSuccessfullyseems to be failing intermittently on Bitrise, but not locally. Here we are adding a timeout and measuring elapsed time to the flaky test to determine if it is timing issue.馃摑 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120)