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

Added test cases for UdpSender.send (#328) #667

Closed
wants to merge 2 commits into from

Conversation

rajovictor
Copy link

@rajovictor rajovictor commented Oct 29, 2019

Signed-off-by: Raja Shekar Reddy Peddireddyy rajovictor@gmail.com

Which problem is this PR solving?

Short description of the changes

  • Tests successful send and MessageTooLarge exception case.
  • Renamed few test cases to reflect what they are actually doing.

Signed-off-by: Raja Peddireddyy <rajovictor@gmail.com>
@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #667      +/-   ##
============================================
- Coverage     89.93%   89.69%   -0.24%     
+ Complexity      569      567       -2     
============================================
  Files            69       69              
  Lines          2086     2086              
  Branches        266      266              
============================================
- Hits           1876     1871       -5     
- Misses          129      133       +4     
- Partials         81       82       +1
Impacted Files Coverage Δ Complexity Δ
...rtracing/internal/reporters/CompositeReporter.java 71.42% <0%> (-28.58%) 6% <0%> (-1%)
...gertracing/internal/reporters/LoggingReporter.java 81.81% <0%> (-9.1%) 4% <0%> (-1%)
...egertracing/internal/reporters/RemoteReporter.java 87.35% <0%> (-2.3%) 7% <0%> (ø)
...ernal/baggage/RemoteBaggageRestrictionManager.java 98.3% <0%> (+3.38%) 11% <0%> (ø) ⬇️

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 46798bd...e5c2e8c. Read the comment docs.

@yurishkuro
Copy link
Member

@guo0693 do you mind reviewing this?

Copy link
Contributor

@guo0693 guo0693 left a comment

Choose a reason for hiding this comment

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

line 212 - 225 seems to be duplicated as line 171 - 187. Can we just have one test: testSendSpanWithOperationName() for two test cases:
operationName, expException?

for( test : testCases) {
  //create span etc using test.operationName
 ...
  if(test.expException) {
    //try send
    //assert exception caught
  } else {
    //assert received spans.
  }
}

@@ -132,7 +138,7 @@ public void testAppend() throws Exception {
}

@Test
public void testFlushSendsSpan() throws Exception {
public void testFlushAppendsSpan() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Feel like original name makes sense, the flush function cause the send of the spans, and server side gets the batch.

Copy link
Author

Choose a reason for hiding this comment

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

Since its appending before Flush, I thought the new name covers the intent well.
Do avoid the duplicate code, I extracted the Span creation into a method.

Signed-off-by: Raja Peddireddyy <rajovictor@gmail.com>
@yurishkuro
Copy link
Member

@rajovictor do these tests add something different than #670? If so, could you please rebase?

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.

UdpSender broken
3 participants