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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

unit test coverage for PubSubClient NATS implementation Publish method #52

Merged
merged 16 commits into from May 21, 2019

Conversation

Projects
None yet
2 participants
@aaslamin
Copy link
Contributor

commented May 16, 2019

Parent issue: aporeto-inc/aporeto#1274

This PR provides close to 馃挴% unit test coverage on the Publish method for the NATS implementation of the PubSubClient interface.

Coming up in future PRs:

  • Unit tests for the NATS Subscribe method
  • Unit tests for NATS Connect method
  • Unit tests for NATS Disconnect method

aaslamin added some commits May 15, 2019

new: add client interface for NATS
Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
new: generate mock for NATS client
Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
new:
鈥 add option for setting the connection retry interval - NATSOptConnectRetryInterval
鈥 add test coverage for NATSOptConnectRetryInterval option

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
new: add stronger assertions to ensure that the generated client id i鈥
鈥 actually a V4 UUID

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
new: add option for overriding the NATS client - particularly useful 鈥
鈥or unit testing

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
new: add test coverage for pubsub nats Publish - scenario: should suc鈥
鈥essfully publish publication

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
new:
鈥 add error type for NoClientError - make it possible for consumers to properly handle errors
鈥 add error type for EncodingError - make it possible for consumers to properly handle errors

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
new: add test coverage for pubsub nats Publish - scenario: should ret鈥
鈥rn a NoClientError error if no NATS client had been connected

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
new: add test coverage for pubsub nats Publish - scenario: should ret鈥
鈥rn an EncodingError error if the publication fails to get encoded

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
new: add test coverage for pubsub nats Publish - scenario: should be 鈥
鈥ble to provide a custom reply validator using the NATSOptPublishRequireAck option

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
new: add test coverage for pubsub nats Publish - scenario: should ret鈥
鈥rn an error if custom response validator fails response message validation

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
new: add test coverage for pubsub nats Publish - scenario: should ret鈥
鈥rn an error if RequestWithContext returns an error

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
new: add test coverage for pubsub nats Publish - scenario: should ret鈥
鈥rn an error if Publish returns an error

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>

@aaslamin aaslamin requested review from primalmotion and t00f May 16, 2019

// verify that client id is a proper V4 UUID
id, err := uuid.FromString(ps.clientID)
So(err, ShouldBeNil)
So(id.Version(), ShouldEqual, uuid.V4)

This comment has been minimized.

Copy link
@aaslamin

aaslamin May 17, 2019

Author Contributor

馃憖 this will prevent someone in the future changing behaviour by not using a v4 UUID for the client ID generation (this assertion will fail).

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

Cover all teh things!

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

This fix needs to go in first: aporeto-inc/elemental#42

type natsPubSub struct {
natsURL string
client *nats.Conn
client NATSClient

This comment has been minimized.

Copy link
@primalmotion

primalmotion May 21, 2019

Member

I don't think this interface needs to be public

This comment has been minimized.

Copy link
@aaslamin

aaslamin May 21, 2019

Author Contributor

Done

@@ -52,6 +60,14 @@ func NATSOptTLS(tlsConfig *tls.Config) NATSOption {
}
}

// NATSOptClient sets the NATS client that will be used
// This is useful for unit testing as you can pass in a mocked NATS client
func NATSOptClient(client NATSClient) NATSOption {

This comment has been minimized.

Copy link
@primalmotion

primalmotion May 21, 2019

Member

this should also be private

This comment has been minimized.

Copy link
@aaslamin

aaslamin May 21, 2019

Author Contributor

Done


"go.aporeto.io/elemental"

This comment has been minimized.

Copy link
@primalmotion

primalmotion May 21, 2019

Member

no new lines here

This comment has been minimized.

Copy link
@aaslamin

aaslamin May 21, 2019

Author Contributor

Done

fix: don't export NATSOption that enables the overriding the NATS client
Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "nats-pubsub-publish-tests",
    "component": "bahamut",
    "pr-id": "52",
    "commit-sha": "d46a71ad26649bd32930ba4f322bf08e798288ed"
  }
]
@codecov

This comment has been minimized.

Copy link

commented May 21, 2019

Codecov Report

Merging #52 into master will increase coverage by 10.85%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #52       +/-   ##
==========================================
+ Coverage   86.55%   97.4%   +10.85%     
==========================================
  Files          28       5       -23     
  Lines        2097     154     -1943     
==========================================
- Hits         1815     150     -1665     
+ Misses        258       2      -256     
+ Partials       24       2       -22
Impacted Files Coverage 螖
publication.go
health_server.go
metrics_prometheus.go
pubsub_local.go
bahamut.go
opentracing.go
websocket_server.go
pinger.go
profiling_server.go
utils.go
... and 11 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 f5cbbe3...aa11546. Read the comment docs.

fix: remove custom error types for now
Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "nats-pubsub-publish-tests",
    "component": "bahamut",
    "pr-id": "52",
    "commit-sha": "aa115467b05afff8a0b14e9be152e1224f0b9509"
  }
]

@primalmotion primalmotion merged commit a04690d into master May 21, 2019

5 of 6 checks passed

functional-tests Submitter: reason: . functional-tests set to pending
Details
built
Details
codecov/patch Coverage not affected when comparing f5cbbe3...aa11546
Details
codecov/project 97.4% (+10.85%) compared to f5cbbe3
Details
functional-tests-trigger Submitter: reason: . functional-tests-trigger set to success
Details
unit-tests
Details

@aaslamin aaslamin deleted the test-coverage-for-nats branch May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.