[SNOW-1881731] Add externalBrowser, oauth, okta, keypair automated authentication tests #1082
Conversation
.github/workflows/parameters/parameters_aws_auth_tests.json.gpg
Outdated
Show resolved
Hide resolved
Snowflake.Data.Tests/AuthenticationTests/AuthConnectionParameters.cs
Outdated
Show resolved
Hide resolved
| return properties; | ||
| } | ||
|
|
||
| public static String SetBaseConnectionParameters(Dictionary<string, string> param) |
There was a problem hiding this comment.
- return string not String
- params - plural form is better
- the name of method is not describing what this method is dooing. Maybe GetBaseConnectionString could be better?
There was a problem hiding this comment.
- Ok
- 'params' is restricted word, I will use 'parameters'
- I will keep GET/SET convention but change ConnectionParameters to 'ConnectionString'
Snowflake.Data.Tests/AuthenticationTests/AuthConnectionParameters.cs
Outdated
Show resolved
Hide resolved
Snowflake.Data.Tests/AuthenticationTests/AuthConnectionParameters.cs
Outdated
Show resolved
Hide resolved
| var parameters = AuthConnectionParameters.GetExternalBrowserConnectionParameters(); | ||
|
|
||
| parameters["user"] = "differentUser"; | ||
| _connectionString = AuthConnectionParameters.SetExternalBrowserConnectionParameters(parameters); |
There was a problem hiding this comment.
why do we use shared _connectionString field?
I we would create connectionString variable in each test, the tests would be independent from each other
There was a problem hiding this comment.
connection string each time is setup to the default value, furthermore it use static methods to be setup
| { | ||
| var parameters = AuthConnectionParameters.GetExternalBrowserConnectionParameters(); | ||
|
|
||
| _connectionString = AuthConnectionParameters.SetExternalBrowserConnectionParameters(parameters); |
There was a problem hiding this comment.
if AuthConnectionParameters.GetExternalBrowserConnectionParameters() method could take BROWSER_RESPONSE_TIMEOUT as a parameter
and if we used instead of SetExternalBrowserConnectionParameters, a dedicated to external browser a generic method which transforms a dictionary into a connectin string then all the methods Set... would be not needed and such things neither _connectionString += "BROWSER_RESPONSE_TIMEOUT=1;";
There was a problem hiding this comment.
The idea behind helper is to initialise integration with minimal number of parameters and to make them common for each kind of integration, it was made that way for each driver.
| namespace Snowflake.Data.Tests.AuthenticationTests | ||
| { | ||
| [NonParallelizable] | ||
| public class KeyPairConnection : SFBaseTest |
There was a problem hiding this comment.
KeyPairConnectionTest ?
| namespace Snowflake.Data.Tests.AuthenticationTests | ||
| { | ||
| [NonParallelizable] | ||
| public class OauthConnection : SFBaseTest |
There was a problem hiding this comment.
OAuthConnectionTest
| namespace Snowflake.Data.Tests.AuthenticationTests | ||
| { | ||
| [NonParallelizable] | ||
| public class OktaConnection : SFBaseTest |
There was a problem hiding this comment.
OktaConnectionTest
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1082 +/- ##
==========================================
- Coverage 86.20% 86.13% -0.08%
==========================================
Files 131 131
Lines 12554 12554
Branches 1290 1290
==========================================
- Hits 10822 10813 -9
- Misses 1414 1423 +9
Partials 318 318 ☔ View full report in Codecov by Sentry. |
Description
Please explain the changes you made here.
This PR adds automated tests for externalBrowser, oauth, okta, keypair authentication methods
Checklist
dotnet test)