Skip to content

Add unit tests for a variety of chart aspects #288

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
Merged
merged 14 commits into from
Mar 1, 2022
Merged

Add unit tests for a variety of chart aspects #288

merged 14 commits into from
Mar 1, 2022

Conversation

@cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Feb 22, 2022 •
edited
Loading

Part of #28

  • correct helm unittest usage with more than one test file
  • Add unit tests for
    • required Labels
    • SecurityContext
    • imagePullSecrets and PullPolicy
    • Resources
    • Pod placement (NodeSelector, Tolerations, Affinity)
    • dnsConfig, dnsPolicy
    • ServiceAccount
    • postStartScript

@cognifloyd cognifloyd self-assigned this Feb 22, 2022
@cognifloyd cognifloyd requested a review from arm4b February 22, 2022 05:07
@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Feb 22, 2022
@pull-request-size pull-request-size bot added size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review. and removed size/L PR that changes 100-499 lines. Requires some effort to review. labels Feb 23, 2022
@pull-request-size pull-request-size bot added size/XXL PR that changes 1000+ lines. You should absolutely split your PR into several. and removed size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review. labels Feb 23, 2022
@cognifloyd cognifloyd changed the title Add unit tests for labels and securityContext Add unit tests for a variety of chart aspects Feb 23, 2022
@cognifloyd
Copy link
Member Author

@armab I added a bunch of tests here. I'll stop adding more to this PR and wait for your review.

cognifloyd
cognifloyd commented Feb 23, 2022
View reviewed changes
Copy link
Member Author

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

The tests reveal a few inconsistencies that we might want to fix in follow-up PRs.

tests/unit/labels_test.yaml
Comment on lines +194 to +200
# ServiceAccount does not have tier or vendor
#- equal:
# path: metadata.labels.tier
# value: backend
#- equal:
# path: metadata.labels.vendor
# value: stackstorm
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe fix this in a follow up

tests/unit/post_start_script_test.yaml
Comment on lines +123 to +126
# st2actionrunner and st2client do not have checksum annotations
# (even though they probably should)
- isNull: &assert_checksum
path: spec.template.metadata.annotations.[checksum/post-start-script]
Copy link
Member Author

Choose a reason for hiding this comment

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

Fix this in a follow up (need to drop the condition since they always have a script)

tests/unit/resources_test.yaml
Comment on lines +90 to +96
# st2client hard codes resources.requests
- equal:
path: spec.template.spec.containers[0].resources.requests
value:
memory: "5Mi"
cpu: "5m"
documentIndex: 12
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should make this configurable in a follow up.

tests/unit/service_account_test.yaml
Comment on lines +110 to +113
# st2client does not allow attaching serviceAccount
- isNull:
path: spec.template.spec.serviceAccountName
documentIndex: 12
Copy link
Member Author

Choose a reason for hiding this comment

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

That's odd. Maybe add st2client.serviceAccount.attach in a follow up.

@cognifloyd cognifloyd mentioned this pull request Feb 23, 2022
Open
arm4b
arm4b approved these changes Feb 25, 2022
View reviewed changes
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Thanks @cognifloyd 👍

I'd also say add a few quick unit tests for the #218 and let's merge it to call that done.

@cognifloyd
Copy link
Member Author

@armab OK. I added tests for stackstorm/sensor-mode=all-sensors-in-one-pod and stackstorm/sensor-mode=one-sensor-per-pod in 3c86fd6. I can add the tests for just the sensor hash-partitioning feature in #218.

Please check out 3c86fd6, and then merge this if you are happy with those tests.

@cognifloyd cognifloyd requested a review from arm4b March 1, 2022 00:38
@arm4b
Copy link
Member

arm4b commented Mar 1, 2022

@cognifloyd It all looks already great. Feel free to merge!

I really meant it would be nice to add tests in the #218 so we can merge that one too, but with st2sensors_test.yaml added here, the coverage will be even better 👍

@cognifloyd cognifloyd merged commit 524c0e1 into master Mar 1, 2022
@cognifloyd cognifloyd deleted the helm-unittest branch March 1, 2022 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arm4b arm4b Awaiting requested review from arm4b

Assignees

@cognifloyd cognifloyd

Labels

CI/CD Helm size/XXL PR that changes 1000+ lines. You should absolutely split your PR into several.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cognifloyd @arm4b