Skip to content
This repository was archived by the owner on Mar 2, 2022. It is now read-only.

Added metrics to SFlux/SMono #56

Merged
merged 1 commit into from
May 30, 2020
Merged

Conversation

rs017991
Copy link
Contributor

I went ahead and copied the docs from the core reactor project, I hope that's okay.

@codecov
Copy link

codecov bot commented May 29, 2020

Codecov Report

Merging #56 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
+ Coverage   80.20%   80.27%   +0.06%     
==========================================
  Files          17       17              
  Lines         576      578       +2     
  Branches        8        5       -3     
==========================================
+ Hits          462      464       +2     
  Misses        114      114              
Impacted Files Coverage Δ
...ain/scala/reactor/core/scala/publisher/SFlux.scala 96.42% <100.00%> (+0.01%) ⬆️
...ain/scala/reactor/core/scala/publisher/SMono.scala 90.30% <100.00%> (+0.05%) ⬆️

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 252956e...4868be7. Read the comment docs.

Copy link
Contributor

@sinwe sinwe left a comment

Choose a reason for hiding this comment

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

I've left some comments.

@@ -1526,6 +1526,11 @@ class SFluxTest extends AnyFreeSpec with Matchers with TableDrivenPropertyChecks
.expectNext(1, 2, 3, 4, 5, 6)
.verifyComplete()
}

".metrics should be a nop since Micrometer is not on the classpath" in {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also add a test with Micrometer on the classpath? You can add the library for test.

@@ -163,6 +163,11 @@ class SMonoTest extends AnyFreeSpec with Matchers with TestSupport with Idiomati
}
}
}

".metrics should be a nop since Micrometer is not on the classpath" in {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as SFluxTest, if you can add Micrometer on test classpath and add the test here that'd be good.

Copy link
Contributor

@sinwe sinwe left a comment

Choose a reason for hiding this comment

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

I understand that adding test for both available and unavailable would need to restructure the whole thing. I'll approve this for now while thinking how to test with Micrometer available on the classpath as well.

@sinwe sinwe merged commit 830f6be into spring-attic:master May 30, 2020
@sinwe
Copy link
Contributor

sinwe commented May 30, 2020

Thank you @rs017991

@rs017991
Copy link
Contributor Author

rs017991 commented Jun 1, 2020

@sinwe after some thought, I'm leaning towards changing the tests to be micrometer-only. I think that would be more valuable.

Testing both scenarios would be overly complex, if even possible.

Will follow up with another PR

@rs017991 rs017991 deleted the metrics branch June 1, 2020 15:43
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants