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

Jump to bottom

chore(deadline): added tests for configure_identity_registration_settings.py #593

Merged
merged 1 commit into from
Oct 8, 2021
Merged

chore(deadline): added tests for configure_identity_registration_settings.py #593

merged 1 commit into from
Oct 8, 2021

Conversation

horsmand
Copy link
Contributor

@horsmand horsmand commented Oct 4, 2021 •
edited
Loading

  1. Wrote a bash script to execute the python unit tests and added a script entry in package.json called testpy to run the bash script. The tests are really lightweight, so I expanded the test target to run them along with the other CDK unit tests.
  2. Wrote unit tests for configure_identity_registration_settings.py. I think coverage is pretty good but likely not 100%

These tests will run after the CDK unit tests are complete. The current output from their run looks like this:

aws-rfdk: Tests successful. Total time (4m12.2s) | /__w/aws-rfdk/aws-rfdk/node_modules/jest/bin/jest.js (4m12.1s)
aws-rfdk: $ ./test-python.sh
aws-rfdk: Running python tests
aws-rfdk: ............
aws-rfdk: ----------------------------------------------------------------------
aws-rfdk: Ran 12 tests in 0.009s
aws-rfdk: OK

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@jusiskin jusiskin added the contribution/core This is a PR that came from AWS. label Oct 4, 2021
@horsmand horsmand mentioned this pull request Oct 4, 2021
Closed
@horsmand horsmand force-pushed the sm_worker_auto_registration_py_tests branch from 26f0ac3 to 80a4bd3 Compare October 4, 2021 17:06
jusiskin
jusiskin requested changes Oct 4, 2021
View reviewed changes
Copy link
Contributor

@jusiskin jusiskin left a comment

Choose a reason for hiding this comment

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

This looks great overall. I just have a few minor suggestions - mostly around improving readability of the tests.

packages/aws-rfdk/lib/deadline/scripts/python/.gitignore Outdated Show resolved Hide resolved
packages/aws-rfdk/test-python.sh Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/deadline/scripts/python/test_configure_identity_registration_settings.py Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/deadline/scripts/python/test_configure_identity_registration_settings.py Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/deadline/scripts/python/test_configure_identity_registration_settings.py Outdated Show resolved Hide resolved
packages/aws-rfdk/lib/deadline/scripts/python/test_configure_identity_registration_settings.py Outdated Show resolved Hide resolved
kozlove-aws
kozlove-aws previously approved these changes Oct 6, 2021
View reviewed changes
Copy link
Contributor

@kozlove-aws kozlove-aws left a comment

Choose a reason for hiding this comment

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

LGTM! Good job.

@horsmand horsmand force-pushed the sm_worker_auto_registration_py_tests branch from 80a4bd3 to e327d92 Compare October 8, 2021 04:31
@horsmand horsmand requested a review from jusiskin October 8, 2021 04:32
kozlove-aws
kozlove-aws previously approved these changes Oct 8, 2021
View reviewed changes
jusiskin
jusiskin requested changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@jusiskin jusiskin left a comment

Choose a reason for hiding this comment

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

Looking good. I have a few readability suggestions, which I've provided code for, but there's still some missing coverage around negative tests that we may want to add.

...s/aws-rfdk/lib/deadline/scripts/python/test/test_configure_identity_registration_settings.py Outdated Show resolved Hide resolved
...s/aws-rfdk/lib/deadline/scripts/python/test/test_configure_identity_registration_settings.py Outdated Show resolved Hide resolved
...s/aws-rfdk/lib/deadline/scripts/python/test/test_configure_identity_registration_settings.py Show resolved Hide resolved
...s/aws-rfdk/lib/deadline/scripts/python/test/test_configure_identity_registration_settings.py Outdated Show resolved Hide resolved
...s/aws-rfdk/lib/deadline/scripts/python/test/test_configure_identity_registration_settings.py Outdated Show resolved Hide resolved
...s/aws-rfdk/lib/deadline/scripts/python/test/test_configure_identity_registration_settings.py Outdated Show resolved Hide resolved
@horsmand horsmand force-pushed the sm_worker_auto_registration_py_tests branch 2 times, most recently from 0315fa5 to 3ae3924 Compare October 8, 2021 18:50
jusiskin
jusiskin requested changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@jusiskin jusiskin left a comment

Choose a reason for hiding this comment

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

Just one minor method name conflict. But otherwise this is great

...s/aws-rfdk/lib/deadline/scripts/python/test/test_configure_identity_registration_settings.py Outdated Show resolved Hide resolved
@horsmand horsmand force-pushed the sm_worker_auto_registration_py_tests branch from 3ae3924 to b7d14b0 Compare October 8, 2021 19:54
@horsmand horsmand requested a review from jusiskin October 8, 2021 19:54
jusiskin
jusiskin approved these changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@jusiskin jusiskin left a comment

Choose a reason for hiding this comment

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

Great work here! This looks good to me

@horsmand horsmand merged commit e73d318 into aws:feature_enable_secret_manager Oct 8, 2021
@horsmand horsmand deleted the sm_worker_auto_registration_py_tests branch October 8, 2021 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kozlove-aws kozlove-aws kozlove-aws approved these changes

@jusiskin jusiskin jusiskin approved these changes

Assignees
No one assigned
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@horsmand @jusiskin @kozlove-aws