[Carry #1990]Fix notification unit test #2133
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
|
Shouldn't this be a new case? I think was testing a failed DNS resolution here. |
|
The comment was unclear. It appeared to only really be testing a pre-http network failure. Assuming DNS (or even route failure) should not really be in scope for the test as it is making too many assumption about the network, which cased the original issue in the first place. If a unit test can easily fail based on a network configuration the test is too fragile. |
If running test behide a proxy, we may get the error code
403 Forbidden which will fail line 135 for the last testcase.
Detail:
```
metrics not as expected: notifications.EndpointMetrics{Pending:0,
Events:0, Successes:4, Failures:0, Errors:0,
Statuses:map[string]int{"307 Temporary Redirect":0, "400 Bad Request":0,
"403 Forbidden":0, "200 OK":4}} !=
notifications.EndpointMetrics{Pending:0, Events:0, Successes:4,
Failures:0, Errors:0, Statuses:map[string]int{"400 Bad Request":0, "200
OK":4, "307 Temporary Redirect":0}}
```
Immediate close will fix that
Signed-off-by: Hu Keping <[email protected]>
Signed-off-by: Derek McGowan <[email protected]>
dmcgowan
force-pushed
the
fix-notification-unit-test
branch
from
9dc4c07
to
efc3209
Compare
Current coverage is 51.10% (diff: 100%)@@ master #2133 diff @@
==========================================
Files 125 125
Lines 11434 11434
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 6987 5843 -1144
- Misses 3559 4845 +1286
+ Partials 888 746 -142
|
|
@dmcgowan So, I wrote that comment. This test was there because the error for a DNS miss was different than a connection error. I don't remember any more details than that. LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If running test behide a proxy, we may get the error code
403 Forbidden which will fail line 135 for the last testcase.
Detail:
Immediate close will fix that
Signed-off-by: Hu Keping [email protected]
Signed-off-by: Derek McGowan [email protected]