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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail Stream Replicator on startup if source or target isn't reachable #183

Closed
wants to merge 3 commits into from
Closed

Fail Stream Replicator on startup if source or target isn't reachable #183

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
adb452b
Fail Stream Replicator on startup if source or target isn't reachable…
TiganeteaRobert Aug 3, 2022
5e9cf7c
added tests that were commented
TiganeteaRobert Aug 12, 2022
dc9ee38
fixed unit test
TiganeteaRobert Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions pkg/source/pubsub/pubsub_source_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ func TestPubSubSource_ReadAndReturnSuccessIntegration(t *testing.T) {
}
}

// newPubSubSource_Failure should fail if we can't reach PubSub, commented out this test until we look into https://github.com/snowplow-devops/stream-replicator/issues/151
/*
// newPubSubSource_Failure should fail if we can't reach PubSub
func TestNewPubSubSource_Failure(t *testing.T) {
if testing.Short() {
t.Skip("skipping integration test")
Expand All @@ -77,7 +76,6 @@ func TestNewPubSubSource_Failure(t *testing.T) {
assert.Nil(pubsubSource)
// This should return an error when we can't connect, rather than proceeding to the Write() function before we hit a problem.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit - would be nice to check for the error message as you have done for other sources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

}
*/

// TestNewPubSubSource_Success tests the typical case of creating a new pubsub source.
func TestNewPubSubSource_Success(t *testing.T) {
Expand Down
12 changes: 7 additions & 5 deletions pkg/source/sqs/sqs_source_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"os"
"path/filepath"
"reflect"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -51,8 +52,7 @@ func TestNewSQSSourceWithInterfaces_Success(t *testing.T) {
assert.Nil(err)
}

// newSQSSourceWithInterfaces should fail if we can't reach SQS, commented out this test until we look into https://github.com/snowplow-devops/stream-replicator/issues/151
/*
// newSQSSourceWithInterfaces should fail if we can't reach SQS
func TestNewSQSSourceWithInterfaces_Failure(t *testing.T) {
// Unlike the success test, we don't require anything to exist for this one
assert := assert.New(t)
Expand All @@ -61,10 +61,12 @@ func TestNewSQSSourceWithInterfaces_Failure(t *testing.T) {

source, err := newSQSSourceWithInterfaces(client, "00000000000", 10, testutil.AWSLocalstackRegion, "nonexistent-queue")

assert.Nil(source)
assert.NotNil(err)
assert.NotNil(source)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the rationale for doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As newSqsSourceWithInterfaces just returns a new sqsSource regardless of the parameters it gets and never errors, the assertion we had there was wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right but the issue this PR is for is to establish behaviour where functions like this one fail when we can't connect. The test is meant to illustrate that desired behaviour - so the test should stay as it was and the behaviour of the function should change in this PR

assert.Nil(err)

err = source.Read(nil)
assert.True(strings.HasPrefix(err.Error(), `Failed to get SQS queue URL`))
Copy link
Collaborator

Choose a reason for hiding this comment

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

If err is nil this will panic - see the rest of the tests in the project on (on master at least) for pattern that avoids this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}
*/

// TODO: When we address https://github.com/snowplow-devops/stream-replicator/issues/151, this test will need to change.
func TestSQSSource_ReadFailure(t *testing.T) {
Expand Down
16 changes: 0 additions & 16 deletions pkg/target/eventhub_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,19 +395,3 @@ func TestNewEventHubTarget_ConnVarsErr(t *testing.T) {
assert.EqualError(err, `Error initialising EventHub client: could not reach Event Hub: dial tcp: lookup test.servicebus.windows.net: no such host`)
assert.Nil(tgt)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was removed as the same behaviour is tested in the unit test above it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like the same behaviour to me...

There are two ways to authenticate against EH - connection string or key value. In the scenario where no valid combination of the required env vars for one of the two methods is present, the client doesn't provide a good debugging experience - so we manually check this in the code. The existing unit tests are for that check.

If the correct env vars are provided, however, we can still fail to connect to the client - it might be unavailable, the credentials provided might be incorrect, etc. So another test should be needed to check this.

Copy link
Contributor Author

@TiganeteaRobert TiganeteaRobert Aug 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I’m having a hard time understanding the issue here, it seems to me that the test that I deleted was not even correct after all. It sets env vars for EVENTHUB_KEY_NAME and EVENTHUB_KEY_VALUE and expects a No valid combination of authentication Env vars found error, which would not be the case. That error would appear if no env vars have been set, either for the connection string or the eventhub key. This possibility is tested above in TestNewEventHubTarget_CredentialsNotFound.

I might be missing something here so please if you could provide a bit more context on this, or even book a quick chat about it it would be really helpful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Taking a second look, you're right, the different possible failures are indeed accounted for. I think I just looked in the wrong place and confused myself - these tests are indeed fine.

// NewEventHubTarget should fail if we can't reach EventHub, commented out this test until we look into https://github.com/snowplow-devops/stream-replicator/issues/151
// Note that when we do so, the above tests will need to be changed to use some kind of mock
/*
func TestNewEventHubTarget_Failure(t *testing.T) {
assert := assert.New(t)

// Test that we can initialise a client with Key and Value
t.Setenv("EVENTHUB_KEY_NAME", "fake")
t.Setenv("EVENTHUB_KEY_VALUE", "fake")

tgt, err := newEventHubTarget(&cfg)
assert.Equal("Error initialising EventHub client: No valid combination of authentication Env vars found. https://pkg.go.dev/github.com/Azure/azure-event-hubs-go#NewHubWithNamespaceNameAndEnvironment", err.Error())
assert.Nil(tgt)
}
*/
10 changes: 3 additions & 7 deletions pkg/target/pubsub_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,17 +286,13 @@ func TestNewPubSubTarget_Success(t *testing.T) {
assert.IsType(PubSubTarget{}, *pubsubTarget)
}

// TestnewPubSubTarget_Failure tests that we fail early when we cannot reach pubsub
// Commented out as this behaviour is not currently instrumented.
// This test serves to illustrate the desired behaviour for this issue: https://github.com/snowplow-devops/stream-replicator/issues/151
/*
func TestnewPubSubTarget_Failure(t *testing.T) {
// TestNewPubSubTarget_Failure tests that we fail early when we cannot reach pubsub
func TestNewPubSubTarget_Failure(t *testing.T) {
assert := assert.New(t)

pubsubTarget, err := newPubSubTarget(`nonexistent-project`, `nonexistent-topic`)

// TODO: Test for the actual error we expect, when we have instrumented failing fast
assert.NotNil(err)
assert.EqualError(err, `Connection to PubSub failed, topic does not exist`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Damn, wish I knew about EqualError before I spent ages wrapping stuff across the project in if statements 🤦

assert.Nil(pubsubTarget)
}
*/