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

Sync internal queues to prevent cleaning up log files too soon in tests #1053

Merged
merged 2 commits into from Mar 14, 2019

Conversation

ffried
Copy link
Member

@ffried ffried commented Mar 14, 2019

New Pull Request Checklist

  • I have read and understood the CONTRIBUTING guide

  • I have read the Documentation

  • I have searched for a similar pull request in the project and found none

  • I have updated this branch with the latest master to avoid conflicts (via merge from master or rebase)

  • I have added the required tests to prove the fix/feature I am adding

  • I have updated the documentation (if necessary)

  • I have run the tests and they pass

  • I have run the lint and it passes (pod lib lint)

Pull Request Description

On the iOS simulator, we would have a race condition where test files would be cleaned up before the tested file logger was able to clean up after itself. This fixes this by syncing DDLog's queue and the file logger's queue before deleting the log files/.

@ffried ffried mentioned this pull request Mar 14, 2019
8 tasks
@ffried ffried added this to the 3.5.2 milestone Mar 14, 2019
@lolgear lolgear self-requested a review March 14, 2019 13:51
Copy link
Contributor

@lolgear lolgear left a comment

Choose a reason for hiding this comment

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

Nice!

Besides, this project has a lot of async/sync code and also has a lot of different async techniques. This technique is a new for me. I don't think about this type of await by double-syncing.

@codecov-io
Copy link

codecov-io commented Mar 14, 2019

Codecov Report

Merging #1053 into master will decrease coverage by 0.62%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1053      +/-   ##
=========================================
- Coverage   39.33%   38.7%   -0.63%     
=========================================
  Files          27      27              
  Lines        3991    3999       +8     
=========================================
- Hits         1570    1548      -22     
- Misses       2421    2451      +30
Impacted Files Coverage Δ
Tests/Tests/DDFileLoggerTests.m 99.07% <100%> (+0.07%) ⬆️
Classes/DDASLLogger.m 13.46% <0%> (-42.31%) ⬇️
Classes/DDOSLogger.m 51.42% <0%> (-22.86%) ⬇️
Classes/Extensions/DDFileLogger+Buffering.m 78.72% <0%> (-1.07%) ⬇️
Tests/Tests/DDLogMessageTests.m 91.83% <0%> (-0.69%) ⬇️
Classes/DDFileLogger.m 58.6% <0%> (+0.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f647c2f...7f7e8ae. Read the comment docs.

@lolgear lolgear merged commit dd86867 into master Mar 14, 2019
@sushichop
Copy link
Member

@ffried
Great!

@sushichop sushichop deleted the bugfix/no_overachieving_tests branch March 15, 2019 12:21
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.

None yet

4 participants