Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Fixes #3301. Fix SearchEngineManagerTest test. #3309

Merged
merged 3 commits into from Jun 10, 2019

Conversation

dector
Copy link
Contributor

@dector dector commented Jun 7, 2019

Issue #3301

Removed useless launch {}.join() construction in test.

I think this test was failing sometimes because we are using coroutine context from runBlockingTest with TestCoroutineDispatcher inside. But joining to job explicitly scheduled to Dispatchers.Default.

Complexity

Trivial (★☆☆)

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

@dector dector requested a review from a team as a code owner June 7, 2019 13:20
Copy link
Contributor

@jhugman jhugman left a comment

Choose a reason for hiding this comment

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

Code looks good. This helps us a lot, thank you.

@jonalmeida
Copy link
Contributor

I'm going to run this a couple of times to see if we get any intermittent failures in CI. When I originally debugged this, it wasn't as easy to repro this locally.

@dector
Copy link
Contributor Author

dector commented Jun 7, 2019

@jonalmeida To reproduce this one can use something like:

launch(Dispatchers.Default) { Thread.sleep(1000) }.join()

SearchEngineManager is totally ok because we are passing (test) coroutine context to it. Using another thread (from Default dispatcher) messes test.

There are few issues in Kotlin coroutines repository for it and fix isn't merged yet.

@codecov
Copy link

codecov bot commented Jun 10, 2019

Codecov Report

Merging #3309 into master will decrease coverage by 0.62%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3309      +/-   ##
============================================
- Coverage     81.76%   81.14%   -0.63%     
+ Complexity     3275     3069     -206     
============================================
  Files           425      405      -20     
  Lines         14278    13574     -704     
  Branches       2044     1894     -150     
============================================
- Hits          11674    11014     -660     
- Misses         1765     1803      +38     
+ Partials        839      757      -82
Impacted Files Coverage Δ Complexity Δ
...ts/service/glean/scheduler/MetricsPingScheduler.kt 88.57% <0%> (-10%) 22% <0%> (ø)
...components/browser/session/action/BrowserAction.kt
.../browser/engine/system/matcher/ReversibleString.kt
.../components/browser/session/ext/StoreExtensions.kt
...nents/browser/engine/system/SystemEngineSession.kt
...ngine/system/permission/SystemPermissionRequest.kt
...onents/browser/engine/system/matcher/UrlMatcher.kt
.../components/browser/engine/system/NestedWebView.kt
...omponents/feature/session/bundling/ext/Snapshot.kt
...s/feature/session/bundling/SessionBundleStorage.kt
... and 27 more

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 8948840...3f10a34. Read the comment docs.

@jonalmeida jonalmeida merged commit a5bf110 into mozilla-mobile:master Jun 10, 2019
@dector dector deleted the fix-3301 branch June 10, 2019 10:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants