Skip to content

Commit

Permalink
[SNOW-1528866] Re-enable some unit tests on Windows (1/N) (#1308)
Browse files Browse the repository at this point in the history
### Changes description
Re-enables a few unit tests on Windows. Issues fixed:
- Gitignored all dirs that start with `.venv`, so we can create a separate `.venv-win` for example
- Keep file iteration sorted to generate deterministic snapshots
- Always emit POSIX paths in snapshots
- Generate test TOML files using `tomlkit` instead of raw strings since windows paths like `C:\Users\..` contain `\U`, which is interpreted as a Unicode escape (dumping using `tomlkit` escapes these properly)
- Cleanup logging handlers between each test to close log file handle
- Update tests with hardcoded `/` in paths to use `os.path.join` or `os.sep`
- Some just worked already (/shrug)
  • Loading branch information
sfc-gh-fcampbell authored Jul 13, 2024
1 parent 426173a commit c8b1d14
Show file tree
Hide file tree
Showing 8 changed files with 56 additions and 45 deletions.
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
__pycache__
.venv
.venv*
*.egg-info
config.ini
credentials
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,9 @@ def process(

collected_output = []
collected_sql_files: List[Path] = []
for py_file, extension_fns in collected_extension_functions_by_path.items():
for py_file, extension_fns in sorted(
collected_extension_functions_by_path.items()
):
sql_file = self.generate_new_sql_file_name(
py_file=py_file,
)
Expand Down Expand Up @@ -326,8 +328,12 @@ def collect_extension_functions(
Path, List[NativeAppExtensionFunction]
] = {}

for src_file, dest_file in bundle_map.all_mappings(
absolute=True, expand_directories=True, predicate=_is_python_file_artifact
for src_file, dest_file in sorted(
bundle_map.all_mappings(
absolute=True,
expand_directories=True,
predicate=_is_python_file_artifact,
)
):
cc.step(
"Processing Snowpark annotations from {}".format(
Expand Down
5 changes: 0 additions & 5 deletions tests/api/test_sanitizers.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,6 @@
from snowflake.cli.api.sanitizers import sanitize_for_terminal
from typer.testing import CliRunner

from tests_common import IS_WINDOWS

if IS_WINDOWS:
pytest.skip("Does not work on Windows", allow_module_level=True)


@pytest.mark.parametrize(
"text, expected",
Expand Down
18 changes: 14 additions & 4 deletions tests/api/test_secure_path.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,22 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import os
import re
import shutil
import stat
from functools import reduce
from pathlib import Path

import pytest
import tomlkit
from snowflake.cli.api import secure_path
from snowflake.cli.api.config import config_init
from snowflake.cli.api.exceptions import DirectoryIsNotEmptyError, FileTooLargeError
from snowflake.cli.api.secure_path import SecurePath
from snowflake.cli.app import loggers

from tests.conftest import clean_logging_handlers
from tests.testing_utils.files_and_dirs import assert_file_permissions_are_strict
from tests_common import IS_WINDOWS

Expand All @@ -36,14 +38,21 @@
def save_logs(snowflake_home):
config = snowflake_home / "config.toml"
logs_path = snowflake_home / "logs"
config.write_text(
"\n".join(["[cli.logs]", "save_logs = true", f'path = "{logs_path}"'])
config_data = dict(
cli=dict(
logs=dict(
save_logs=True,
path=str(logs_path),
)
)
)
tomlkit.dump(config_data, config.open("w"))
config_init(config)
loggers.create_loggers(False, False)

yield logs_path

clean_logging_handlers()
shutil.rmtree(logs_path)


Expand Down Expand Up @@ -159,7 +168,8 @@ def test_open_read(temp_dir, save_logs):

def test_navigation():
p = SecurePath("a/b/c")
assert str(p / "b" / "c" / "d" / "e") == 'SecurePath("a/b/c/b/c/d/e")'
pathstring = reduce(os.path.join, ["a", "b", "c", "b", "c", "d", "e"])
assert str(p / "b" / "c" / "d" / "e") == f'SecurePath("{pathstring}")'
assert str(p.parent.parent) == 'SecurePath("a")'
assert type(p.absolute()) is SecurePath

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -545,8 +545,8 @@
# name: test_process_with_collected_functions.1
'''
===== Contents of: output/deploy/__generated/__generated.sql =====
EXECUTE IMMEDIATE FROM '/__generated/stagepath/main.sql';
EXECUTE IMMEDIATE FROM '/__generated/stagepath/data.sql';
EXECUTE IMMEDIATE FROM '/__generated/stagepath/main.sql';

'''
# ---
Expand All @@ -560,7 +560,7 @@
RETURNS int
LANGUAGE PYTHON
RUNTIME_VERSION=3.11
IMPORTS=('/', '/stagepath/data.py', '/stagepath/extra_import1.zip', '/stagepath/some_dir_str', '/stagepath/withslash.py', '@dummy_stage_str')
IMPORTS=('/path/to/import1.py', '/path/to/import2.zip', '/stagepath/data.py')
PACKAGES=('package_one==1.0.2', 'package_two', 'snowflake-snowpark-python')
EXTERNAL_ACCESS_INTEGRATIONS=('integration_one', 'integration_two')
SECRETS=('key1'=secret_one, 'key2'=integration_two)
Expand All @@ -583,7 +583,7 @@
RETURNS int
LANGUAGE PYTHON
RUNTIME_VERSION=3.11
IMPORTS=('/path/to/import1.py', '/path/to/import2.zip', '/stagepath/main.py')
IMPORTS=('/', '/stagepath/data.py', '/stagepath/extra_import1.zip', '/stagepath/main.py', '/stagepath/some_dir_str', '/stagepath/withslash.py', '@dummy_stage_str')
PACKAGES=('package_one==1.0.2', 'package_two', 'snowflake-snowpark-python')
EXTERNAL_ACCESS_INTEGRATIONS=('integration_one', 'integration_two')
SECRETS=('key1'=secret_one, 'key2'=integration_two)
Expand Down
6 changes: 3 additions & 3 deletions tests/nativeapp/codegen/snowpark/test_python_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,6 @@
from tests.testing_utils.files_and_dirs import pushd, temp_local_dir
from tests_common import IS_WINDOWS

if IS_WINDOWS:
pytest.skip("Requires further refactor to work on Windows", allow_module_level=True)

PROJECT_ROOT = Path("/path/to/project")

# --------------------------------------------------------
Expand Down Expand Up @@ -275,6 +272,9 @@ def test_edit_setup_script_with_exec_imm_sql_noop(os_agnostic_snapshot):
)


@pytest.mark.skipif(
IS_WINDOWS, reason="Symlinks on Windows are restricted to Developer mode or admins"
)
def test_edit_setup_script_with_exec_imm_sql_symlink(os_agnostic_snapshot):
manifest_contents = dedent(
f"""\
Expand Down
6 changes: 4 additions & 2 deletions tests/nativeapp/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,11 @@ def assert_dir_snapshot(root: Path, os_agnostic_snapshot) -> None:
# Verify that each file under the directory matches expectations
for path in all_paths:
if path.is_file():
snapshot_contents = f"===== Contents of: {path} =====\n"
snapshot_contents = f"===== Contents of: {path.as_posix()} =====\n"
snapshot_contents += path.read_text(encoding="utf-8")
assert snapshot_contents == os_agnostic_snapshot
assert (
snapshot_contents == os_agnostic_snapshot
), f"\nExpected:\n{os_agnostic_snapshot}\nGot:\n{snapshot_contents}"


def create_native_app_project_model(
Expand Down
46 changes: 22 additions & 24 deletions tests/test_logs.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@
from __future__ import annotations

import logging
import os
import shutil
from contextlib import contextmanager
from pathlib import Path
from typing import Optional

import pytest
import tomlkit
from snowflake.cli.api.config import config_init
from snowflake.cli.api.exceptions import InvalidLogsConfiguration
from snowflake.cli.app import loggers
Expand All @@ -29,9 +31,6 @@
from tests.testing_utils.files_and_dirs import assert_file_permissions_are_strict
from tests_common import IS_WINDOWS

if IS_WINDOWS:
pytest.skip("Requires further refactor to work on Windows", allow_module_level=True)


@pytest.fixture
def setup_config_and_logs(snowflake_home):
Expand All @@ -49,36 +48,32 @@ def _setup_config_and_logs(
logs_path = snowflake_home / "custom" / "logs"

config_path = snowflake_home / "config.toml"
config_path.write_text(
"\n".join(
x
for x in [
"[connections]",
"",
"[cli.logs]",
f'path = "{logs_path}"' if use_custom_logs_path else None,
(
f"save_logs = {str(save_logs).lower()}"
if save_logs is not None
else None
),
f'level = "{level}"' if level else None,
]
if x is not None
)
)
log_config_data: dict[str, str | bool] = {}
config_data = dict(connections={}, cli=dict(logs=log_config_data))
if use_custom_logs_path:
log_config_data["path"] = str(logs_path)
if save_logs is not None:
log_config_data["save_logs"] = save_logs
if level:
log_config_data["level"] = level
tomlkit.dump(config_data, config_path.open("w"))

config_path.chmod(0o700)

# Make sure we start without any leftovers
shutil.rmtree(logs_path, ignore_errors=True)
clean_logging_handlers()
shutil.rmtree(logs_path, ignore_errors=True)

# Setup loggers
config_init(config_path)
loggers.create_loggers(verbose=verbose, debug=debug)
assert len(_list_handlers()) == (2 if save_logs else 1)

yield logs_path

# After the test, logging handlers still have open file handles
# Close everything so we can delete the log file
clean_logging_handlers()
shutil.rmtree(logs_path, ignore_errors=True)

return _setup_config_and_logs
Expand Down Expand Up @@ -132,7 +127,7 @@ def test_logs_section_appears_in_fresh_config_file(temp_dir):
config_init(config_file)
assert config_file.exists() is True
assert '[cli.logs]\nsave_logs = true\npath = "' in config_file.read_text()
assert '/logs"\nlevel = "info"' in config_file.read_text()
assert f'{os.sep}logs"\nlevel = "info"' in config_file.read_text()


def test_logs_saved_by_default(setup_config_and_logs):
Expand Down Expand Up @@ -194,7 +189,7 @@ def test_log_level_is_not_overriden_by_verbose_flag(capsys, setup_config_and_log


def test_stdout_log_level_remains_error(capsys, setup_config_and_logs):
with setup_config_and_logs(save_logs=True, level="debug") as logs_path:
with setup_config_and_logs(save_logs=True, level="debug"):
print_log_messages()
captured = capsys.readouterr()
assert_log_level(captured.out + captured.err, expected_level="error")
Expand All @@ -211,6 +206,9 @@ def test_incorrect_log_level_in_config(setup_config_and_logs):
)


@pytest.mark.skipif(
IS_WINDOWS, reason="Permissions for new files aren't strict in Windows"
)
def test_log_files_permissions(setup_config_and_logs):
with setup_config_and_logs(save_logs=True) as logs_path:
print_log_messages()
Expand Down

0 comments on commit c8b1d14

Please sign in to comment.