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.

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

Add unit tests for cookbook EBS related scripts #2445

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

judysng
Copy link
Contributor

@judysng judysng commented Sep 5, 2023

Description of changes

  • Wrote unit tests for functions ec2_dev_2_volid and manageVolume because these files lacked unit tests
  • Separated the main function in ec2_dev_2_volid into multiple functions
  • Fixed typo in manageVolume

Tests

  • Added tests for most functions in the two files except for some functions that just called the other tested functions

Checklist

  • Make sure you are pointing to the right branch.
  • If you're creating a patch for a branch other than develop add the branch name as prefix in the PR title (e.g. [release-3.6]).
  • Check all commits' messages are clear, describing what and why vs how.
  • Make sure to have added unit tests or integration tests to cover the new/modified code.
  • Check if documentation is impacted by this change.

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

test/unit/ec2_udev_rules/test_ec2_dev_2_volid.py Outdated Show resolved Hide resolved
test/unit/ec2_udev_rules/test_ec2_dev_2_volid.py Outdated Show resolved Hide resolved
return dev


def parse_proxy_config():
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we not testing this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided not to test this function because it seemed to just be calling lots of other functions without much logic in that function itself. Let me know if you think I should test this too though!

test/unit/ec2_udev_rules/test_ec2_dev_2_volid.py Outdated Show resolved Hide resolved
test/unit/ec2_udev_rules/test_manage_volume.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Patch coverage: 34.28% and project coverage change: +5.51% 🎉

Comparison is base (71c252b) 70.49% compared to head (607f7d6) 76.01%.

❗ Current head 607f7d6 differs from pull request most recent head 721e484. Consider uploading reports for the commit 721e484 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2445      +/-   ##
===========================================
+ Coverage    70.49%   76.01%   +5.51%     
===========================================
  Files           13       13              
  Lines         1864     1876      +12     
===========================================
+ Hits          1314     1426     +112     
+ Misses         550      450     -100     
Flag Coverage Δ
unittests 76.01% <34.28%> (+5.51%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...nt/files/default/ec2_udev_rules/ec2_dev_2_volid.py 64.00% <28.00%> (+34.76%) ⬆️
...nment/files/default/ec2_udev_rules/manageVolume.py 79.85% <50.00%> (+59.41%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aws aws deleted a comment from himani2411 Sep 6, 2023
@judysng judysng requested a review from himani2411 September 6, 2023 21:38
Copy link
Contributor

@enrico-usai enrico-usai left a comment

Choose a reason for hiding this comment

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

In general it's useful to split this kind of changes in multiple small commits, e.g.:

  • create parse_proxy_config function
  • create get_device_volume_id
  • fix typo in manageVolume function
  • add unit tests for xxx function

This simplifies the review.

Instead you should fixup the second commit you created, this doesn't have any meaning by itself, it's a fix of the previous commit so they should be merged together. See https://www.jetbrains.com/idea/guide/tips/interactive-rebase/

Anyway you did a great job, you can see here how your patch is increasing the coverage of about 6%.

@judysng judysng force-pushed the unittests branch 4 times, most recently from 3ee0387 to 7afcf86 Compare September 8, 2023 17:12
@judysng judysng requested a review from enrico-usai September 8, 2023 17:12
@judysng judysng force-pushed the unittests branch 2 times, most recently from 607f7d6 to c29cf84 Compare September 12, 2023 15:11
…eparated ec2_dev_2_volid main function into multiple functions, fixed typo in manageVolume

Signed-off-by: Judy Ng <[email protected]>
@judysng judysng merged commit 05ebede into aws:develop Sep 13, 2023
25 checks passed
@judysng judysng deleted the unittests branch September 13, 2023 15:25
hgreebe pushed a commit to hgreebe/aws-parallelcluster-cookbook that referenced this pull request Nov 13, 2023
…eparated ec2_dev_2_volid main function into multiple functions, fixed typo in manageVolume (aws#2445)

Signed-off-by: Judy Ng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants