In 908 build dev testing framework

[jonavellecuerdo]

What does this PR do?

Scope out a workflow that allows a developer to easily test the application.

Helpful background context

When testing, the main quirk is that the host name for the Data Warehouse is different when you connect to the Data Warehouse outside of AWS. Please reach out to me if you need help setting you .env file.

How can a reviewer manually see the effects of these changes?

Follow the instructions in the README to run the Data Warehouse connection test
by running the application in a Docker container on your local machine.

1. Follow the instructions in the README under "Running the application on your local machine"
2. Follow the instructions in the README under "With Docker".
3. Run make run-connection-tests-dev.

Includes new or updated dependencies?

NO

Developer

• README is updated to reflect all changes as needed
• All related Jira tickets are linked in commit message(s)

Code Reviewer

• The commit message is clear and follows our guidelines
  (not just this pull request message)
• There are appropriate tests covering any new functionality
• The documentation has been updated to reflect all changes or is not needed
• The changes have been verified
• New dependencies are appropriate or there were no changes

[ehanson8]

A few thoughts

[ehanson8]

I would use the language you used with step 4 in With Docker instead of referencing the ECS command

[jonavellecuerdo]

Applied the change.

[ehanson8]

I'm not sure about using dev here if this is in fact a local run

[ghukill]

Agreed, had the same thought.

[jonavellecuerdo]

Hmm, I changed the name of the Make command to run-connection-tests-locally and updated the the instructions to read:

Run any make commands for testing the application. In the Makefile, the names of relevant make commands will contain the suffix ['-locally' | '-stage'].

Let me know what you think!

[ehanson8]

I think this should include a note to skip make publish-dev since we just need to run make dist-dev to build the container locally right?

[ghukill]

Also agreed. Will save for a more general comment, but I think this is a good example where some of these instructions deviate for purely local, docker testing vs testing the deployed container in Dev1.

[jonavellecuerdo]

Added a sentence about skipping the step to publish the Docker container.

[ghukill]

Instead of leaving inline comments, and in anticipation of discussing later, going to break it out into some higher level comments.

As we've batted around a bit, I think there is some blurring between "tests" and "docker development conveniences" (for lack of a better term).

Some of the big value I'm seeing here is the ability to run a docker container locally, of the app, and have it perform aspects of the full, normal application logic that help a developer as they work. Specifically, the ability to redirect the generated XML to a local file instead of writing to FTP.

This is particularly important given the Oracle DB library dependencies, that don't work on Apple Silicon. The docker container is critical for seeing actual output, from actual data in the data warehouse.

When I see a CLI option like --run_connection_tests it does what I expect: it's running some connection tests, printing those results, and then bailing.

But while the option --output_file=/output/people.xml is very helpful, and did see the XML that was written to the container and mounted to my host machine, it also sent some SNS logs and did more work than I needed. As I think you suggested @jonavellecuerdo, maybe another flag to prevent that from happening could be helpful.

More granular, noticed some things in the README that may need updating:

• env vars
  • think LOG_LEVEL and SENTRY_DSN are both required for docker runs
  • SNS_TOPIC is an ARN; consider renaming to SNS_TOPIC_ARN?
  • good example of "full" vs "limited" functionality: maybe not require SYMPLECTIC_FTP_JSON if I'm only writing XML to a local file?
  • AWS_DEFAULT_REGION was also required, but not explicitly stated, and not always a default AWS env var
• am I misunderstanding, but wouldn't a VPN connection always be needed for data warehouse queries?

All that said, I think it's a great step forward in the ability to see / test the output without writing to FTP. Looking forward to discussing more, but feel like it might just come down to the README explaining what "convenience" features are available for development, while being clear they aren't really "tests" in the traditional sense.

[jonavellecuerdo]

@ghukill A few responses to your comments:

Addressed:

• LOG_LEVEL and SENTRY_DSN: These aren't needed for local development. When it isn't set:
  • LOG_LEVEL defaults to "INFO";
  • SENTRY_DSN defaults to "None".
    It was throwing an error because of a typo I made! Thank you for pointing this out! See change in carbon/config.py. These vars should truly be optional now!
• AWS_DEFAULT_REGION: See updated Step 1 of "Running the application on your local machine".

To-dos:

• Renaming SNS_TOPIC: I will reach out to Christopher and ask if it would be possible to make this change! I do agree that the name change would be beneficial when trying to test the application.
• Optional SE environment variables: Need to reflect on this a little more. 🤔 💭
• Optional SNS logging

[jonavellecuerdo]

Hi @ghukill and @ehanson8 ! Please review the changes from the last two commits. Namely, these commits involve (as discussed):

• Additional updates to the README

  • Note: I converted the 'Required Env' section to use a commented code block instead. I know we're discussing additional methods we can take to create a .env template, but would it be okay if we save that work for a separate story related to the Config... work?

• Updates to SNS logging tests

  • Note: I was originally trying to create separate tests for the start, success, and fail options for sns_log(...), however, because of the how stubbed_sns_client works (i.e., "Responses are returned first in, first out.", I had to encapsulate all tests into one.

Thank you again and let me know if you have additional thoughts/questions! :)

[ehanson8]

• Note: I converted the 'Required Env' section to use a commented code block instead. I know we're discussing additional methods we can take to create a .env template, but would it be okay if we save that work for a separate story related to the Config... work?

I think that's fine to hold off on a template until further Config discussions

[ehanson8]

• Note: I was originally trying to create separate tests for the start, success, and fail options for sns_log(...), however, because of the how stubbed_sns_client works (i.e., "Responses are returned first in, first out.", I had to encapsulate all tests into one.

I think it's fine but this could be addressed by changing the fixture's scope=session since I don't think it's good practice to stack up multiple function calls in a single unit test. But given the first-in-first-out nature, I don't think the test is really testing much other than that the fixture is returning messages so whether it's 1 or 3 doesn't matter than much. That's the nature of some tests with mocks, they aren't the most insightful but better than nothing. @ghukill Your thoughts?

[ehanson8]

Looks good but I'll defer to @ghukill for final approval since this involves first passes on some future best practices

[ehanson8]

@ghukill Your thoughts on this formatting for comments in copy/paste .env section? I think you did it differently for timdex-simulator and I think it's worth standardizing early on but I don't have strong feelings on either approach, either works for me!

[ghukill]

I'm good with this format. Personally, I'm in support of comments above env vars, as that would support multi-line comments if we ever need them. Yes, it's busy, and takes up space, but that's okay by me.

A couple minor nit picks:

• I'd suggest we try and float common env vars to the top, like WORKSPACE
• probably don't need a comment on WORKSPACE, as it is common to most repos, and worry those comments would drift / conflict / confuse
• FWIW, and maybe not applicable here, but I'd value logical grouping over alphanumeric

At the end of the day, I'd really just imagine you're new to the codebase: what do you want communicated to you when you first look at this? I'd hope for really important and/or really obvious ones at the top, likely without comments. Then little sections of interelated ones, with comments as needed.

Just as an example, for this repo me knee-jerk expectation might be:

WORKSPACE= "dev"

# type of feed, either "people" or "articles"
FEED_TYPE="people"

# JSON formatted string of key/value pairs for the MIT Data Warehouse connection
DATAWAREHOUSE_CLOUDCONNECTOR_JSON='{"USER": "<VALID_DATAWAREHOUSE_USERNAME>", "PASSWORD": "<VALID_DATAWAREHOUSE_PASSWORD>", "HOST": "<VALID_DATAWAREHOUSE_HOST>", "PORT": "<VALID_DATAWAREHOUSE_PORT>", "PATH": "<VALID_DATAWAREHOUSE_ORACLE_SID>", "CONNECTION_STRING": "<VALID_DATAWAREHOUSE_CONNECTION_STRING>"}'

# JSON formatted string of key/value pairs for Symplectic FTP server connection
SYMPLECTIC_FTP_JSON='{"SYMPLECTIC_FTP_HOST": "<VALID_ELEMENTS_FTP_HOST>", "SYMPLECTIC_FTP_PORT": "<VALID_ELEMENTS_FTP_PORT>", "SYMPLECTIC_FTP_USER": "<VALID_ELEMENTS_FTP_USER>", "SYMPLECTIC_FTP_PASS": "<VALID_ELEMENTS_FTP_PASSWORD>"}'

# full XML filepath that is uploaded to the Symplectic FTP server
SYMPLECTIC_FTP_PATH="<FTP_FILE_DIRECTORY>/<FEED_TYPE>.xml"

# SNS topic ARN used for sending email notifications
SNS_TOPIC="arn:..."

• just stylistic and subjective, but try and keep them short and snappy, easily scannable
• avoiding capital letters at the beginning, makes the comments stand-out from the UPPERCASE env vars
• floated the symplectic FTP connection under database, as they are related

[jonavellecuerdo]

Fixed this!

[ehanson8]

I like this refactored block

[ehanson8]

Isn't this test doing more than this now? I'd make it more generic

[jonavellecuerdo]

Fixed the name! Thank you for catching this.

[ghukill]

• Note: I was originally trying to create separate tests for the start, success, and fail options for sns_log(...), however, because of the how stubbed_sns_client works (i.e., "Responses are returned first in, first out.", I had to encapsulate all tests into one.

I think it's fine but this could be addressed by changing the fixture's scope=session since I don't think it's good practice to stack up multiple function calls in a single unit test. But given the first-in-first-out nature, I don't think the test is really testing much other than that the fixture is returning messages so whether it's 1 or 3 doesn't matter than much. That's the nature of some tests with mocks, they aren't the most insightful but better than nothing. @ghukill Your thoughts?

I agree, and think that's well summarized. Without digging in too deeply, it seems okay to me to think of this test as testing how the sns_log function accepts parameters and which code path it follows based on that. It doesn't tell us much about whether or not that function is called when we expect in the normal flow of things, but perhaps that is tested somewhere else.

Theoretically -- as @ehanson8 pointed out -- this fixture could be scoped to the test level and could even accept the expected request/response payloads as arguments to re-initialize it each time.

But... unsure if worth the trouble here.

[ghukill]

Looks good to me!

Agreed with @ehanson8 on most all fronts. My comments are pretty stylistic and subjective, please feel free to consider or disregard. Overall, I think it's looking geat.

[ghukill]

Nice! So the returned receipt is enought to assert on it looks like?

[ghukill]

I find myself still tripping on this Makefile command. When I see "local" here, that makes me think it's attempting to connect to a local Oracle instance.

What about just run-connection-tests? I would assume, then, that it's the .env file that drives what context is getting tested.

Perhaps the "local" -- and I was present for the conversation and agreed at the time -- was us trying to suggest to the developer that it's using docker? If so, what about run-connection-tests-via-docker? It's long... but perhaps it indicates what is happening?

[jonavellecuerdo]

Hmm, we've been using the phrase "run-connection-tests-stage" for "running the connection tests via an ECS task in Stage-Workloads".

I updated the Makefile commands to run-connection-tests-with-docker and run-connection-tests-with-ecs-stage.

Let me know what you think!

[ghukill]

Ah, good point. Sorry for the all the back and forth on this. I guess it's understandable, as the naming kind of needles what the real goal is.

I like this one, run-connection-tests-with-ecs-stage:

• running connection tests
• hitting ECS directly
• stage environment is hardcoded into make command, so makes sense

Now the other, run-connection-tests-with-docker, if I'm summarizing my understanding:

• runs connection tests
• which Oracle DB, and which Symplectic FTP endpoint, are driven by .env file
• looking at command, can see quite clearly it's running docker container on local machine

So... I think I like it? If I, as a developer, look at the command in the Makefile I can see the .env file getting mounted.

My follow-up question, am I missing it, or is this command documented in the README? If not, what about a small comment in the Makefile, something along the lines of:

# Run connection tests from local docker instance, driven by Oracle DB and Symplectic FTP configs from env vars

[jonavellecuerdo]

Added a comment! :)

[ghukill]

I'm good with this format. Personally, I'm in support of comments above env vars, as that would support multi-line comments if we ever need them. Yes, it's busy, and takes up space, but that's okay by me.

A couple minor nit picks:

• I'd suggest we try and float common env vars to the top, like WORKSPACE
• probably don't need a comment on WORKSPACE, as it is common to most repos, and worry those comments would drift / conflict / confuse
• FWIW, and maybe not applicable here, but I'd value logical grouping over alphanumeric

At the end of the day, I'd really just imagine you're new to the codebase: what do you want communicated to you when you first look at this? I'd hope for really important and/or really obvious ones at the top, likely without comments. Then little sections of interelated ones, with comments as needed.

Just as an example, for this repo me knee-jerk expectation might be:

WORKSPACE= "dev"

# type of feed, either "people" or "articles"
FEED_TYPE="people"

# JSON formatted string of key/value pairs for the MIT Data Warehouse connection
DATAWAREHOUSE_CLOUDCONNECTOR_JSON='{"USER": "<VALID_DATAWAREHOUSE_USERNAME>", "PASSWORD": "<VALID_DATAWAREHOUSE_PASSWORD>", "HOST": "<VALID_DATAWAREHOUSE_HOST>", "PORT": "<VALID_DATAWAREHOUSE_PORT>", "PATH": "<VALID_DATAWAREHOUSE_ORACLE_SID>", "CONNECTION_STRING": "<VALID_DATAWAREHOUSE_CONNECTION_STRING>"}'

# JSON formatted string of key/value pairs for Symplectic FTP server connection
SYMPLECTIC_FTP_JSON='{"SYMPLECTIC_FTP_HOST": "<VALID_ELEMENTS_FTP_HOST>", "SYMPLECTIC_FTP_PORT": "<VALID_ELEMENTS_FTP_PORT>", "SYMPLECTIC_FTP_USER": "<VALID_ELEMENTS_FTP_USER>", "SYMPLECTIC_FTP_PASS": "<VALID_ELEMENTS_FTP_PASSWORD>"}'

# full XML filepath that is uploaded to the Symplectic FTP server
SYMPLECTIC_FTP_PATH="<FTP_FILE_DIRECTORY>/<FEED_TYPE>.xml"

# SNS topic ARN used for sending email notifications
SNS_TOPIC="arn:..."

• just stylistic and subjective, but try and keep them short and snappy, easily scannable
• avoiding capital letters at the beginning, makes the comments stand-out from the UPPERCASE env vars
• floated the symplectic FTP connection under database, as they are related

[ghukill]

I like this description. While "SNS Logging" makes me immediately think it might be cranking out lots of logging to SNS, this makes it clear it's really just emails, occassionally.

[ghukill]

• Note: I was originally trying to create separate tests for the start, success, and fail options for sns_log(...), however, because of the how stubbed_sns_client works (i.e., "Responses are returned first in, first out.", I had to encapsulate all tests into one.

I think it's fine but this could be addressed by changing the fixture's scope=session since I don't think it's good practice to stack up multiple function calls in a single unit test. But given the first-in-first-out nature, I don't think the test is really testing much other than that the fixture is returning messages so whether it's 1 or 3 doesn't matter than much. That's the nature of some tests with mocks, they aren't the most insightful but better than nothing. @ghukill Your thoughts?

I agree, and think that's well summarized. Without digging in too deeply, it seems okay to me to think of this test as testing how the sns_log function accepts parameters and which code path it follows based on that. It doesn't tell us much about whether or not that function is called when we expect in the normal flow of things, but perhaps that is tested somewhere else.

Theoretically -- as @ehanson8 pointed out -- this fixture could be scoped to the test level and could even accept the expected request/response payloads as arguments to re-initialize it each time.

But... unsure if worth the trouble here.

For the record, continued to dig in, and may have misspoke a bit. But, discussing in a call vs writing here.

[jonavellecuerdo]

@ghukill Applied the suggestions you shared with me and they work well!

[ghukill]

Given that work to make the SNS fixtures more flexible, what about breaking them out into individual tests now?

Related: now that you have the ability to simulate 1) start, and 2) fail SNS messages, what about an unhappy path test that triggers the failed SNS message when something goes wrong during normal app flow? Unless I'm missing that test if it already exists.

Otherwise, looking good!

[ghukill]

I think it's nice these are broken into [start, succes] and [start, fail]. But wonder if it's worth going a step further and just have each tested? Something along the lines of

• stubbed_sns_client_start (fixture) --> test_sns_log_publishes_status_message_start
• stubbed_sns_client_success (fixture) --> test_sns_log_publishes_status_message_success
• stubbed_sns_client_fail (fixture) --> test_sns_log_publishes_status_message_fail

In that case, I think it's still worth keeping the stubbed_sns_client_start_success and stubbed_sns_client_start_fail fixtures, as they are helpful for testing the actual app flow and logic.

[jonavellecuerdo]

@ghukill Thank you for all your help and suggestions to improve testing! Please see the latest commit for updates to tests related to SNS logging and failed runs.

[ghukill]

Looks good to me! Nice work again on all this testing exploration and extending. I think the ability to test a start --> fail for the app flow is good to have.

[ghukill]

Did a little test here and have a suggestion. As we discussed, it feels odd to stub this with start / fail messages, when really you don't anticipate the stubber to get called at all!

Creating an "empty" stubber appears to work given the new pattern:

@pytest.fixture
def stubbed_sns_client_empty():
    sns_client, stubber = setup_stubbed_sns_client([])
    with stubber:
        yield sns_client

Then you can replace stubbed_sns_client_start_fail with stubbed_sns_client_empty.

[ghukill]

I'm understanding how my comment above was incorrect. Looks like you've got a test for a more accurate representation that happens:

1. app throws exception
2. sns_log() invoked to notify about failure
3. stubber is ready for this failure scenario

So it would not bypass the SNS logging entirely. Nice work!

[ghukill]

Nice find with the side_effect