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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix auth tests for windows. #697

Merged
merged 2 commits into from
Jul 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 21 additions & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import functools
import os
import platform

import pytest as pytest
from pyfakefs.fake_filesystem_unittest import Patcher
Expand Down Expand Up @@ -38,7 +39,11 @@ def wrapper(*args, **kwargs):
with pytest.raises(ValueError) as info:
func(*args, **kwargs)
exception_str = str(info.value)
exception_str = exception_str.replace(__tests__ + '/', '')
if platform.system() == 'Windows':
exception_str = exception_str.replace(__tests__ + '\\', '')
exception_str = exception_str.replace('\\', '/')
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little bit weird since it means we will have to change all \ to / in test assertions, which is why I preferred customizing the message itself depending on the OS. Not a big deal though, as these test cases change rarely and only one has a \\ in the exception message now.

else:
exception_str = exception_str.replace(__tests__ + '/', '')
assert msg in exception_str

return wrapper
Expand All @@ -57,3 +62,18 @@ def fake_fs():
patcher.fs.add_real_directory(test_data_path)

yield patcher.fs # This will return a fake file system


def set_home(monkeypatch, path):
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that you don't need to change the direction of the slashes here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find anything on internet regarding this. But maybe it fixes the path before setting the variable.

if platform.system() == 'Windows':
monkeypatch.setenv('USERPROFILE', __tests__ + path)
else:
monkeypatch.setenv('HOME', __tests__ + path)


def set_az_path(monkeypatch):
if platform.system() == 'Windows':
monkeypatch.setenv('Path', __tests__ + "\\testdata\\windows\\")
monkeypatch.setenv('COMSPEC', 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with this variable, what is it used for? Do we only need to set this environment variable when configuring Path?

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 basically points to the command line interpreter by default windows program run on cmd. But we are running a powershell script to mock the az so we need to run it on powershell.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah understood, and this is only needed for tests that run our mock az command.

else:
monkeypatch.setenv('PATH', __tests__ + "/testdata:/bin")
46 changes: 23 additions & 23 deletions tests/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
# In case of editing this file, make sure the change is propagated to all Databricks SDK codebases
from databricks.sdk.core import Config

from .conftest import __tests__, raises
from .conftest import __tests__, raises, set_az_path, set_home
Copy link
Contributor

Choose a reason for hiding this comment

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

Is __tests__ still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in line 219 of test_auth.py, We are pointing the PATH to some garbage place such that It will not find the az command. I am currently not passing any argument related to path in set_az_path. Because in rest of the cases the path is fixed. thats why I needed the __tests__. what are your suggestions regarding this?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's alright, more of a nit not to import things that aren't used, but if it is still used I just missed it. We can leave as-is.


default_auth_base_error_message = \
"default auth: cannot configure default credentials, " \
Expand Down Expand Up @@ -121,27 +121,27 @@ def test_config_config_file(monkeypatch):

@raises(f"{default_auth_base_error_message}. Config: host=https://x")
def test_config_config_file_skip_default_profile_if_host_specified(monkeypatch):
monkeypatch.setenv('HOME', __tests__ + '/testdata')
set_home(monkeypatch, '/testdata')
cfg = Config(host='x')


@raises(default_auth_base_error_message)
def test_config_config_file_with_empty_default_profile_select_default(monkeypatch):
monkeypatch.setenv('HOME', __tests__ + '/testdata/empty_default')
set_home(monkeypatch, '/testdata/empty_default')
Config()


def test_config_config_file_with_empty_default_profile_select_abc(monkeypatch):
monkeypatch.setenv('DATABRICKS_CONFIG_PROFILE', 'abc')
monkeypatch.setenv('HOME', __tests__ + '/testdata/empty_default')
set_home(monkeypatch, '/testdata/empty_default')
cfg = Config()

assert cfg.auth_type == 'pat'
assert cfg.host == 'https://foo'


def test_config_pat_from_databricks_cfg(monkeypatch):
monkeypatch.setenv('HOME', __tests__ + '/testdata')
set_home(monkeypatch, '/testdata')
cfg = Config()

assert cfg.auth_type == 'pat'
Expand All @@ -150,7 +150,7 @@ def test_config_pat_from_databricks_cfg(monkeypatch):

def test_config_pat_from_databricks_cfg_dot_profile(monkeypatch):
monkeypatch.setenv('DATABRICKS_CONFIG_PROFILE', 'pat.with.dot')
monkeypatch.setenv('HOME', __tests__ + '/testdata')
set_home(monkeypatch, '/testdata')
cfg = Config()

assert cfg.auth_type == 'pat'
Expand All @@ -161,7 +161,7 @@ def test_config_pat_from_databricks_cfg_dot_profile(monkeypatch):
f"{default_auth_base_error_message}. Config: token=***, profile=nohost. Env: DATABRICKS_CONFIG_PROFILE")
def test_config_pat_from_databricks_cfg_nohost_profile(monkeypatch):
monkeypatch.setenv('DATABRICKS_CONFIG_PROFILE', 'nohost')
monkeypatch.setenv('HOME', __tests__ + '/testdata')
set_home(monkeypatch, '/testdata')
Config()


Expand All @@ -171,7 +171,7 @@ def test_config_pat_from_databricks_cfg_nohost_profile(monkeypatch):
def test_config_config_profile_and_token(monkeypatch):
monkeypatch.setenv('DATABRICKS_CONFIG_PROFILE', 'nohost')
monkeypatch.setenv('DATABRICKS_TOKEN', 'x')
monkeypatch.setenv('HOME', __tests__ + '/testdata')
set_home(monkeypatch, '/testdata')
Config()


Expand All @@ -181,7 +181,7 @@ def test_config_config_profile_and_token(monkeypatch):
def test_config_config_profile_and_password(monkeypatch):
monkeypatch.setenv('DATABRICKS_CONFIG_PROFILE', 'nohost')
monkeypatch.setenv('DATABRICKS_USERNAME', 'x')
monkeypatch.setenv('HOME', __tests__ + '/testdata')
set_home(monkeypatch, '/testdata')
Config()


Expand All @@ -194,8 +194,8 @@ def test_config_azure_pat():


def test_config_azure_cli_host(monkeypatch):
monkeypatch.setenv('HOME', __tests__ + '/testdata/azure')
monkeypatch.setenv('PATH', __tests__ + '/testdata:/bin')
set_home(monkeypatch, '/testdata/azure')
set_az_path(monkeypatch)
cfg = Config(host='https://adb-123.4.azuredatabricks.net', azure_workspace_resource_id='/sub/rg/ws')

assert cfg.auth_type == 'azure-cli'
Expand All @@ -208,14 +208,14 @@ def test_config_azure_cli_host(monkeypatch):
)
def test_config_azure_cli_host_fail(monkeypatch):
monkeypatch.setenv('FAIL', 'yes')
monkeypatch.setenv('HOME', __tests__ + '/testdata/azure')
monkeypatch.setenv('PATH', __tests__ + '/testdata:/bin')
set_home(monkeypatch, '/testdata/azure')
set_az_path(monkeypatch)
cfg = Config(azure_workspace_resource_id='/sub/rg/ws')


@raises(f"{default_auth_base_error_message}. Config: azure_workspace_resource_id=/sub/rg/ws")
def test_config_azure_cli_host_az_not_installed(monkeypatch):
monkeypatch.setenv('HOME', __tests__ + '/testdata/azure')
set_home(monkeypatch, '/testdata/azure')
monkeypatch.setenv('PATH', __tests__ + '/whatever')
cfg = Config(azure_workspace_resource_id='/sub/rg/ws')

Expand All @@ -224,14 +224,14 @@ def test_config_azure_cli_host_az_not_installed(monkeypatch):
"validate: more than one authorization method configured: azure and pat. Config: token=***, azure_workspace_resource_id=/sub/rg/ws"
)
def test_config_azure_cli_host_pat_conflict_with_config_file_present_without_default_profile(monkeypatch):
monkeypatch.setenv('HOME', __tests__ + '/testdata/azure')
monkeypatch.setenv('PATH', __tests__ + '/testdata:/bin')
set_home(monkeypatch, '/testdata/azure')
set_az_path(monkeypatch)
cfg = Config(token='x', azure_workspace_resource_id='/sub/rg/ws')


def test_config_azure_cli_host_and_resource_id(monkeypatch):
monkeypatch.setenv('HOME', __tests__ + '/testdata')
monkeypatch.setenv('PATH', __tests__ + '/testdata:/bin')
set_home(monkeypatch, '/testdata')
set_az_path(monkeypatch)
cfg = Config(host='https://adb-123.4.azuredatabricks.net', azure_workspace_resource_id='/sub/rg/ws')

assert cfg.auth_type == 'azure-cli'
Expand All @@ -241,8 +241,8 @@ def test_config_azure_cli_host_and_resource_id(monkeypatch):

def test_config_azure_cli_host_and_resource_i_d_configuration_precedence(monkeypatch):
monkeypatch.setenv('DATABRICKS_CONFIG_PROFILE', 'justhost')
monkeypatch.setenv('HOME', __tests__ + '/testdata/azure')
monkeypatch.setenv('PATH', __tests__ + '/testdata:/bin')
set_home(monkeypatch, '/testdata/azure')
set_az_path(monkeypatch)
cfg = Config(host='https://adb-123.4.azuredatabricks.net', azure_workspace_resource_id='/sub/rg/ws')

assert cfg.auth_type == 'azure-cli'
Expand All @@ -255,8 +255,8 @@ def test_config_azure_cli_host_and_resource_i_d_configuration_precedence(monkeyp
)
def test_config_azure_and_password_conflict(monkeypatch):
monkeypatch.setenv('DATABRICKS_USERNAME', 'x')
monkeypatch.setenv('HOME', __tests__ + '/testdata/azure')
monkeypatch.setenv('PATH', __tests__ + '/testdata:/bin')
set_home(monkeypatch, '/testdata/azure')
set_az_path(monkeypatch)
cfg = Config(host='https://adb-123.4.azuredatabricks.net', azure_workspace_resource_id='/sub/rg/ws')


Expand All @@ -265,7 +265,7 @@ def test_config_azure_and_password_conflict(monkeypatch):
)
def test_config_corrupt_config(monkeypatch):
monkeypatch.setenv('DATABRICKS_CONFIG_PROFILE', 'DEFAULT')
monkeypatch.setenv('HOME', __tests__ + '/testdata/corrupt')
set_home(monkeypatch, '/testdata/corrupt')
Config()


Expand Down
22 changes: 11 additions & 11 deletions tests/test_auth_manual_tests.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
from databricks.sdk.core import Config

from .conftest import __tests__
from .conftest import set_az_path, set_home


def test_azure_cli_workspace_header_present(monkeypatch):
monkeypatch.setenv('HOME', __tests__ + '/testdata/azure')
monkeypatch.setenv('PATH', __tests__ + '/testdata:/bin')
set_home(monkeypatch, '/testdata/azure')
set_az_path(monkeypatch)
resource_id = '/subscriptions/123/resourceGroups/abc/providers/Microsoft.Databricks/workspaces/abc123'
cfg = Config(auth_type='azure-cli',
host='https://adb-123.4.azuredatabricks.net',
Expand All @@ -15,8 +15,8 @@ def test_azure_cli_workspace_header_present(monkeypatch):


def test_azure_cli_user_with_management_access(monkeypatch):
monkeypatch.setenv('HOME', __tests__ + '/testdata/azure')
monkeypatch.setenv('PATH', __tests__ + '/testdata:/bin')
set_home(monkeypatch, '/testdata/azure')
set_az_path(monkeypatch)
resource_id = '/subscriptions/123/resourceGroups/abc/providers/Microsoft.Databricks/workspaces/abc123'
cfg = Config(auth_type='azure-cli',
host='https://adb-123.4.azuredatabricks.net',
Expand All @@ -25,8 +25,8 @@ def test_azure_cli_user_with_management_access(monkeypatch):


def test_azure_cli_user_no_management_access(monkeypatch):
monkeypatch.setenv('HOME', __tests__ + '/testdata/azure')
monkeypatch.setenv('PATH', __tests__ + '/testdata:/bin')
set_home(monkeypatch, '/testdata/azure')
set_az_path(monkeypatch)
monkeypatch.setenv('FAIL_IF', 'https://management.core.windows.net/')
resource_id = '/subscriptions/123/resourceGroups/abc/providers/Microsoft.Databricks/workspaces/abc123'
cfg = Config(auth_type='azure-cli',
Expand All @@ -36,8 +36,8 @@ def test_azure_cli_user_no_management_access(monkeypatch):


def test_azure_cli_fallback(monkeypatch):
monkeypatch.setenv('HOME', __tests__ + '/testdata/azure')
monkeypatch.setenv('PATH', __tests__ + '/testdata:/bin')
set_home(monkeypatch, '/testdata/azure')
set_az_path(monkeypatch)
monkeypatch.setenv('FAIL_IF', 'subscription')
resource_id = '/subscriptions/123/resourceGroups/abc/providers/Microsoft.Databricks/workspaces/abc123'
cfg = Config(auth_type='azure-cli',
Expand All @@ -47,8 +47,8 @@ def test_azure_cli_fallback(monkeypatch):


def test_azure_cli_with_warning_on_stderr(monkeypatch):
monkeypatch.setenv('HOME', __tests__ + '/testdata/azure')
monkeypatch.setenv('PATH', __tests__ + '/testdata:/bin')
set_home(monkeypatch, '/testdata/azure')
set_az_path(monkeypatch)
monkeypatch.setenv('WARN', 'this is a warning')
resource_id = '/subscriptions/123/resourceGroups/abc/providers/Microsoft.Databricks/workspaces/abc123'
cfg = Config(auth_type='azure-cli',
Expand Down
56 changes: 56 additions & 0 deletions tests/testdata/windows/az.ps1
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
#!/usr/bin/env pwsh

if ($env:WARN) {
Write-Error "WARNING: $env:WARN"
}

if ($env:FAIL -eq "yes") {
Write-Error "This is just a failing script."
exit 1
}

if ($env:FAIL -eq "logout") {
Write-Error "No subscription found. Run 'az account set' to select a subscription."
exit 1
}

if ($env:FAIL -eq "corrupt") {
Write-Output "{accessToken: ..corrupt"
exit
}

param (
[string[]]$Args
)

foreach ($arg in $Args) {
if ($arg -eq $env:FAIL_IF) {
Write-Output "Failed"
exit 1
}
}

try {
$EXP = (Get-Date).AddSeconds($env:EXPIRE -as [int])
} catch {
$expireString = $env:EXPIRE
$expireString = $expireString -replace "S", "seconds"
$expireString = $expireString -replace "M", "minutes"
$EXP = (Get-Date).AddSeconds($expireString -as [int])
}

if (-not $env:TF_AAD_TOKEN) {
$TF_AAD_TOKEN = "..."
} else {
$TF_AAD_TOKEN = $env:TF_AAD_TOKEN
}

$expiresOn = $EXP.ToString("yyyy-MM-dd HH:mm:ss")

Write-Output "{
`"accessToken`": `"$TF_AAD_TOKEN`",
`"expiresOn`": `"$expiresOn`",
`"subscription`": `"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee`",
`"tenant`": `"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee`",
`"tokenType`": `"Bearer`"
}"
Loading