Skip to content

Conversation

@cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Jan 28, 2021

Add tests for all actions.

To do this, we reuse the hvac testing infra to setup consul/vault for our tests.

Also, we drop the broken python2.7 tests and update the CI config to use the latest from StackStorm-Exchange/ci.

This depends on This demonstrates how StackStorm-Exchange/ci#101 will behave by running the setup_testing_env.sh script immediately following dependencies.

@cognifloyd
Copy link
Member Author

I dropped 2.7 tests due to: StackStorm/community#40 (comment)

3.6 tests are failing because StackStorm-Exchange/ci#101 hasn't been merged yet.

Since the 2.7 job was dropped, we need to save things in the 3.6 job so
that it is available during the deploy step.

Also add some safety bits that are in the StackStorm-Exchange/ci and
should eventually go to all packs.
In [1] the default path was changed from ${HOME}/bin to
${HOME}/.local/bin which is compatible with both CI and local
development, so drop the hacky workaround.

[1] hvac/hvac@56ade59
@cognifloyd cognifloyd requested a review from nmaludy February 3, 2021 02:52
Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

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

I would suggest using super() where we can. Here's a write up from Raymond Hettinger (famous Pythonista) about super().

TL;DR: It leads to less code churn as parent classes change.

Other than that, this looks great!

We still have to have two calls, because HvacIntegrationTestCase does
not call super(). However, we can call super() for it, so do that
instead of the old manual method.
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.

Fascinating. I wasn't sure how to deal with calling super() when one of the parents didn't also call super(). Still using super(SuperCallLessClass, self) is a nicer work around. I've pushed the changes and adjusted the comments to help future me understand what's going on.

@cognifloyd cognifloyd requested a review from blag February 3, 2021 23:23
Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

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

One small mistake needs to be fixed.

Co-authored-by: blag <blag@users.noreply.github.com>
Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for taking this on!

@blag blag merged commit aac0c37 into StackStorm-Exchange:master Feb 4, 2021
@blag blag mentioned this pull request Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants