Introduce Functional Options for MsSQL Connector #1089
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
BradleyBoutcher
commented
Jan 14, 2020
| // ServerPreloginResponse => map[uint8][]byte{} | ||
| // ServerLoginResponse => &mssql.LoginResponse{} | ||
| // BackendConn => nil | ||
| // BackendConn => nil |
There was a problem hiding this comment.
Quote @diverdane
Do we want to use 'options' here instead of 'setters', per Johah's previous comment, or is this used intentionally to distinguish this from ConnectorOptions?
diverdane
reviewed
Jan 15, 2020
internal/plugin/connectors/tcp/mssql/production_options_test.go
Outdated
Show resolved
Hide resolved
internal/plugin/connectors/tcp/mssql/production_options_test.go
Outdated
Show resolved
Hide resolved
- NewSingleUseConnectorWithOptions allows custom injection of dependencies without needing to specify all of them - ConnectorOptions struct captures all the configuration options for a SingleUseConnector - ConnectorOption is the 'functional option' complement to ConnectorOptions - Implemented
- Introduce tests that cover the 'production' options - dependencies we will use during production. - Covers each option specified in the singleuseconnector - Introduces mock options structure used for unit testing, including a function which returns default options
- Unit tests now make use of the t.Run structure, where each test can be ran from within a single connect function - Introduces helper functions for testing around failure - Makes use of the new functional options feature in the connector
BradleyBoutcher
force-pushed
the
functional-options
branch
2 times, most recently
from
7bde982 to
ccdac7e
Compare
SingleUseConnector now has types.ConnectorOptions as an embedded type, allowing us to skip an additional constructor method NewSingleUseConnectorWithOptions has been moved to the connector_test.go
NewSingleUseConnectorWithOptions is now package private in connector_test.go Ordered chronologically the tests in prod_options_test Made DefaultConnectorOptions a variable in options.go
- WriteLogin Succeeds - WriteLogin Fails - ReadLogin Succeeds - ReadLogin Fails
This change adds a test case for a failing readPrelogin call
- WritePreloginSucceeds test
- The production version of WritePreloginWithPacketType
succeeds when all others succeed
- We receive the prelogin fields from the go-mssqldb driver
- We modify the prelogin fields before sending them to the user
- WritePreloginFails test
- Error is returned from connect
- Error is written to client buffer
- Error written to log
BradleyBoutcher
force-pushed
the
functional-options
branch
from
ccdac7e to
8e072c5
Compare
diverdane
approved these changes
Jan 16, 2020
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.
What does this PR do (include background context, if relevant)?
This PR is focused on improving the foundation of our current MsSQL tests, reducing code reuse and time needed to build new unit tests for the connector. It additionally includes a division between tests for our connector, and tests for our production options.
This is a re-uploaded PR of @doodlesbykumbi's work. All credit should go to him.
What ticket does this PR close?
Connected to:
#1010
#1073
Where should the reviewer start?
internal/plugin/connectors/tcp/mssql/connector_test.go
What is the status of the manual tests?
Have you run the following manual tests to verify existing functionality continues to function as expected?
NA
If this feature does not have any/sufficent automated tests, have you created/updated a folder in
test/manualthat includes:NA
Links to open issues for related automated integration and unit tests
NA
Links to open issues for related documentation (in READMEs, docs, etc)
NA
Screenshots (if appropriate)
NA