JSON Templates Tests, S3 Events Support, and Moar Unit Tests #188

jacknagz · 2017-06-20T00:23:41Z

to @ryandeivert
cc @airbnb/streamalert-maintainers

Changes

Move format_lambda_test_record into the cli.helpers package: This is an effort to share this method across tests (for main/handler, which will come soon). I also removed the nesting of helper methods within a CLIHelpers class since that paradigm wasn't being adopted.
Modularize terraform_generate: All cluster generation was in one massive method before, this has been broken out into multiple methods for easier management.
Unit testing: Adds initial unit tests to the terraform_generate code, there are more to add, they will come later.
Moar unit testing: Get our coverage to 100% for the rule processor config module.

* modularize tf generate methods * fix bug in main.tf region loading * coverage for tf_generate methods

ryandeivert · 2017-06-20T00:25:00Z

nit - typo: moar --> more 🤣

jacknagz · 2017-06-20T00:27:24Z

wow it's not a typo @ryandeivert

coveralls · 2017-06-20T00:37:48Z

Coverage increased (+0.8%) to 79.32% when pulling 3e03747 on jacknaglieri-s3-events-json-clusters into 8f086dd on master.

jacknagz · 2017-06-20T01:03:04Z

Related: #176

ryandeivert

Great additions overall! Much needed stuff in here. A few comments in line.

ryandeivert · 2017-06-20T00:27:37Z

test/unit/stream_alert_rule_processor/test_rules_engine.py

+        }
+        # doing this because after kinesis_data is read in, types are casted per the schema
+        for alert in alerts:
+            assert_equal(set(alert['record'].keys()), set(kinesis_data.keys()))


Import and use assert_list_equal here instead of casting to set. Not essential, but makes what is being tested more clear and avoid casting

ryandeivert · 2017-06-20T00:28:59Z

test/unit/stream_alert_rule_processor/test_config.py

+def test_load_config_invalid():
+    """Config Validator - Load Config - Invalid"""
+    m = mock_open()
+    with patch('__builtin__.open', m, create=True):


👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 👏 🎬

we should remove the argument to load_config (it was originally added for tests)

ryandeivert · 2017-06-20T00:31:15Z

test/unit/stream_alert_cli/test_terraform_generate.py

+    """Test class for the Terraform Cluster Generating"""
+
+    @classmethod
+    def setup_class(cls):


setup_class and teardown_class class are not actually required for nose to pick up a class and process tests within it. In fact teardown_class only gets called if setup_class is present, but if there is nothing in either they should be omitted.

ryandeivert · 2017-06-20T00:49:26Z

stream_alert_cli/helpers.py

-            return False
+    try:
+        subprocess.check_call(runner_args, stdout=stdout_option, cwd=cwd)
+    except subprocess.CalledProcessError as e:


Change var name e here to something... not one character long

ryandeivert · 2017-06-20T00:53:49Z

stream_alert_cli/helpers.py

-                cwd (string): A path to execute commands from
-                error_message (string): Message to show if command fails
-                quiet (boolean): Whether to show command output or hide it
+def run_command(cls, runner_args, **kwargs):


cls should be removed with refactor

ryandeivert · 2017-06-20T01:05:06Z

test/unit/stream_alert_cli/test_terraform_generate.py

+            }
+        }
+
+    def teardown(self):


again not necessary

ryandeivert · 2017-06-20T01:06:01Z

test/unit/stream_alert_cli/test_terraform_generate.py

+
+    def test_generate_stream_alert(self):
+        """CLI - Terraform Generate stream_alert Module"""
+        pass


where's the test bruh

added a TODO

ryandeivert · 2017-06-20T01:09:15Z

test/unit/stream_alert_cli/test_terraform_generate.py

+
+    def test_generate_cluster_advanced(self):
+        """CLI - Terraform Generate 'Advanced' Cluster"""
+        config = self.config


by reassigning to config here, you're essentially just creating a pointer to the self.config object anyway. omit this and just pass config=self.config in the generate_cluster call. the self.config is re-created in setup with each new method test anyway.

yeah it was just left in after a replace, I know how pointers work ;)

JUST CHECKIN

ryandeivert · 2017-06-20T01:10:18Z

test/unit/stream_alert_cli/test_terraform_generate.py

+    def test_generate_main(self):
+        """CLI - Terraform Generate Main"""
+        init = False
+        config = self.config


see comment on https://github.com/airbnb/streamalert/pull/188/files#diff-486860095dd40d500eadff4c0a278b6eR362

ryandeivert · 2017-06-20T01:10:30Z

test/unit/stream_alert_cli/test_terraform_generate.py

+
+    def test_generate_cluster_test(self):
+        """CLI - Terraform Generate 'Test' Cluster"""
+        config = self.config


see comment on https://github.com/airbnb/streamalert/pull/188/files#diff-486860095dd40d500eadff4c0a278b6eR362

ryandeivert · 2017-06-20T01:20:35Z

test/unit/stream_alert_cli/test_terraform_generate.py

+import base64
+import json
+
+from nose.tools import assert_equal, assert_not_equal


assert_not_equal is unused here

ryandeivert · 2017-06-20T01:21:08Z

test/unit/stream_alert_cli/test_terraform_generate.py

+                    'id': 'test',
+                    'modules': {
+                        'cloudwatch_monitoring': {
+                          'enabled': True


Weird indentation on this line.

ryandeivert · 2017-06-20T01:21:20Z

test/unit/stream_alert_cli/test_terraform_generate.py

+                    'id': 'advanced',
+                    'modules': {
+                        'cloudwatch_monitoring': {
+                          'enabled': True


Another weird indentation here.

it's not weird, it's alt-proper

ryandeivert

A few more comments

ryandeivert · 2017-06-20T01:25:29Z

stream_alert_cli/helpers.py

+    default_error_message = "An error occurred while running: {}".format(
+        ' '.join(runner_args)
+    )
+    error_message = kwargs.get('error_message', default_error_message)


Seems like error_message is unused :hmm:

Looks like this little fella is still hangin around

it's being used now.. check below

ryandeivert · 2017-06-20T01:26:04Z

stream_alert_cli/helpers.py

+
+    elif service == 'kinesis':
+        if compress:
+            kinesis_data = base64.b64encode(zlib.compress(data))


zlib import is missing here

ryandeivert · 2017-06-20T01:26:40Z

stream_alert_cli/helpers.py

+
+    if service == 's3':
+        # Set the S3 object key to a random value for testing
+        test_record['key'] = ('{:032X}'.format(random.randrange(16**32)))


import random is missing after migrating this function to helpers

interesting, when testing it locally, it must have never made it into this if body

I actually just checked to see why this passed integration tests - none of the rules we have in the integration tests are configured with "service": "s3". There are also none from kinesis configured to use compress, so that's also why that's not failing.

We should either add more encompassing integration tests or we should (need to) add cli unit tests that would have caught this.

ryandeivert · 2017-06-20T01:27:27Z

stream_alert_cli/test.py

@@ -306,77 +305,6 @@ def test_rule(rule_name, test_record, formatted_record):

        return alerts, expected_alert_count

-    @staticmethod


Remember to remove zlib and random from imports in this file now.

I'm going to pylint the whole cli module

ryandeivert · 2017-06-20T01:30:28Z

stream_alert_cli/terraform_generate.py

+
+    account = config['global']['account']
+    prefix = account['prefix']
+    logging_bucket = '{}.streamalert.s3-logging'.format(


prefix, logging_bucket, & account_id are unused variables

#188 (comment)

ryandeivert · 2017-06-20T01:31:36Z

stream_alert_cli/terraform_generate.py

+    """
+    logging_bucket = '{}.streamalert.s3-logging'.format(
+        config['global']['account']['prefix'])
+    firehose_suffix = config['clusters'][cluster_name]['modules']['kinesis']['firehose']['s3_bucket_suffix']


V long line. make this something like:

firehose_suffix = config['clusters'][cluster_name]['modules'] \ ['kinesis']['firehose']['s3_bucket_suffix']

There are a few others like this pylint picks up.

#188 (comment)

ryandeivert · 2017-06-20T01:33:26Z

test/unit/stream_alert_rule_processor/test_config.py

+            conf_logs.write('test logs string that will throw an error')
+        with open('conf/sources.json', 'w') as conf_sources:
+            conf_sources.write('test sources string that will throw an error')
+        config = load_config()


You can just call the load_config here and not assign to variable since it's unused.

jacknagz · 2017-06-20T18:35:28Z

PTAL @ryandeivert

coveralls · 2017-06-20T18:37:03Z

Coverage increased (+0.8%) to 79.32% when pulling aba4870 on jacknaglieri-s3-events-json-clusters into 8f086dd on master.

ryandeivert · 2017-06-20T19:13:17Z

stream_alert_cli/helpers.py

-    except subprocess.CalledProcessError as e:
-        LOGGER_CLI.error('Return Code %s - %s', e.returncode, e.cmd)
+    except subprocess.CalledProcessError as err:
+        LOGGER_CLI.error('%s\n%s', error_message)


Why are there two %s format strings in this logging message but only one arg?

nm looks like this got changed in the next commit.

ryandeivert · 2017-06-20T19:15:38Z

stream_alert_cli/helpers.py

    kms_client = boto3.client('kms', region_name=region)
    response = kms_client.encrypt(KeyId=alias,
                                  Plaintext=data)

    return response['CiphertextBlob']


-def _make_lambda_package():
+def make_lambda_package():


This one isn't used outside of this helper file (not imported elsewhere) and could be kept private. It's only used by the create_lambda_function function.

ryandeivert · 2017-06-20T19:22:05Z

stream_alert_cli/helpers.py

+    default_error_message = "An error occurred while running: {}".format(
+        ' '.join(runner_args)
+    )
+    error_message = kwargs.get('error_message', default_error_message)


Looks like this little fella is still hangin around

ryandeivert · 2017-06-20T19:23:56Z

@jacknagz some comments

jacknagz · 2017-06-20T21:53:47Z

again, PTAL @ryandeivert

coveralls · 2017-06-20T22:39:46Z

Coverage increased (+0.8%) to 79.32% when pulling 9482faa on jacknaglieri-s3-events-json-clusters into 5ec5109 on master.

Jack Naglieri added 3 commits June 19, 2017 17:23

[cli][tests] unit tests for terraform generate code

30238a5

* modularize tf generate methods * fix bug in main.tf region loading * coverage for tf_generate methods

[test][cli] move format_record into helpers and fix flakey test

5a228aa

[test][config] additional coverage to the rule proc config

3e03747

jacknagz force-pushed the jacknaglieri-s3-events-json-clusters branch from 33310b6 to 3e03747 Compare June 20, 2017 00:23

jacknagz added this to the 1.4.0 milestone Jun 20, 2017

ryandeivert requested changes Jun 20, 2017

View reviewed changes

ryandeivert reviewed Jun 20, 2017

View reviewed changes

[pr] ryandeivert's feedback from #188

7c7d1dc

ryandeivert reviewed Jun 20, 2017

View reviewed changes

[linting] pylint and autopep8 on cli + tests

32326a4

jacknagz force-pushed the jacknaglieri-s3-events-json-clusters branch from aba4870 to 32326a4 Compare June 20, 2017 21:53

Merge branch 'master' into jacknaglieri-s3-events-json-clusters

9482faa

airbnb deleted a comment from coveralls Jun 20, 2017

ryandeivert approved these changes Jun 20, 2017

View reviewed changes

jacknagz merged commit d203bbf into master Jun 20, 2017

jacknagz deleted the jacknaglieri-s3-events-json-clusters branch June 20, 2017 22:44

jacknagz added build/test cli terraform labels Jul 6, 2017

		@@ -306,77 +305,6 @@ def test_rule(rule_name, test_record, formatted_record):

		return alerts, expected_alert_count

		@staticmethod

JSON Templates Tests, S3 Events Support, and Moar Unit Tests #188

JSON Templates Tests, S3 Events Support, and Moar Unit Tests #188

Conversation

jacknagz commented Jun 20, 2017

Changes

ryandeivert commented Jun 20, 2017

jacknagz commented Jun 20, 2017

coveralls commented Jun 20, 2017

jacknagz commented Jun 20, 2017

ryandeivert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryandeivert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacknagz commented Jun 20, 2017

coveralls commented Jun 20, 2017 • edited by ryandeivert Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryandeivert commented Jun 20, 2017

jacknagz commented Jun 20, 2017

coveralls commented Jun 20, 2017

coveralls commented Jun 20, 2017 •

edited by ryandeivert

Loading