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

Yaml tests #5

Merged
merged 11 commits into from
Aug 6, 2018
Merged

Yaml tests #5

merged 11 commits into from
Aug 6, 2018

Conversation

Lowercases
Copy link
Member

@Lowercases Lowercases commented May 25, 2018

Add the ability to create blueprint tests in the form of a .yaml file, with a format similar to stacker's configuration.

This adds the stacker_blueprints.test.YamlDirTestGenerator class, which looks for .yaml files in the same directory (this by default, can be changed) and generates a nosetests' test for each "test stack" inside (example: https://github.com/cloudtools/stacker_blueprints/blob/yaml-tests/tests/test_dynamodb.yaml).

In order to use it outside of stacker_blueprints, so that tests can be created outside of the tree, it can be subclassed with minimal effort (example: https://github.com/cloudtools/stacker_blueprints/blob/yaml-tests/tests/__init__.py). When subclassing, some of the behaviour can be changed (yaml filename, yaml tests locations, test baseclass -- details in the docstring).

We've been using this internally at remind with our own internal tests -- this would allow us to use this class appropiately subclassed now. LMK of any suggestions.


Update 7/26: After the implementation of YamlDirTestGenerator was migrated to stacker, import it from there. This PR is now a migration of some tests to yaml format.

Adds the YamlDirTestGenerator testing generator class,
which can be used to load tests from .yaml files in a directory.

Adds an example of its use in tests/.

The base class has a name that doesn't trigger testing
by nosetests, so it can be overloaded.
Add the option for the class extending YamlDirTestGenerator
to specify differents search paths for yaml files.
Migrate these to .yaml format as an example of its use.
@phobologic
Copy link
Member

Is this still a WIP @Lowercases or is it ready for review? Also, this is awesome. Is there anyway to include the framework for this in the base stacker? (I haven't looked at the code yet)

@Lowercases
Copy link
Member Author

@phobologic I left it as WIP since it's still open to commentary/suggestions, but the code is ready for review. We've tested it internally and it's working fine.

As for if it should be included in stacker, I'll leave that at your judgement -- in theory it could go anywhere since it just needs to be extended from the target testing directory.

@phobologic
Copy link
Member

@Lowercases ok cool - usually I only use WIP to indicate that I'm still actively working on it, and it's not ready for review - after all, all PRs are open to commentary/suggestions :) I'll take a look now!

Copy link
Member

@phobologic phobologic left a comment

Choose a reason for hiding this comment

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

Really awesome - I think we should make this a part of the base stacker.

from glob import glob


class YamlDirTestGenerator():
Copy link
Member

Choose a reason for hiding this comment

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

This should be subclassed from object

try:
config = parse_config(test.read())
except Exception as e:
raise Exception("Error loading %s: %s" % (f, e))
Copy link
Member

Choose a reason for hiding this comment

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

Should this instead print an error and raise the original exception? I think the way you have it here won't provide the full stack trace (though i could be wrong, let me know).

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it just makes sense not to catch the error; since the stack trace will print what the issue is in any case. A "C"-ism from me :)

@@ -0,0 +1,125 @@
import os.path
Copy link
Member

Choose a reason for hiding this comment

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

Ok, I really like this - I think this would be better if it were included in stacker itself (maybe in https://github.com/cloudtools/stacker/blob/master/stacker/blueprints/testutil.py), and then we imported it here. Also, it'd be awesome to have some docs for this there: http://stacker.readthedocs.io/en/latest/blueprints.html#testing-blueprints

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I agree -- I'll move the code into stacker then, creating a new PR. I'll leave this open in the meantime.

Copy link
Member

Choose a reason for hiding this comment

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

So awesome. Really looking forward to that!

@Lowercases Lowercases changed the title Yaml tests [WIP] Yaml tests May 26, 2018
@Lowercases
Copy link
Member Author

Lowercases commented Jun 4, 2018

I've migrated the class to stacker and created a PR there: cloudtools/stacker#606

I remain not entirely convinced this patch belongs there, though -- some reasons I can think we could benefit more from having it here:

  • The actual yaml tests will live here in stacker_blueprints (some examples in this same branch); so as these evolve the testing class may evolve as well -- it makes sense for them to share PRs instead of living in separate but concurrent PRs.
  • I believe we want to give this project visibility (blueprints should end here), so this could be a good way for "suggesting" private codebases to use stacker_blueprints. We could reference it still in the docs at stacker.

@phobologic
Copy link
Member

Thanks @Lowercases - I think my thinking is that if someone wants to use stacker, but doesn't necessarily need any of the blueprints, we shouldn't force them to download the blueprints library just to get the awesome functionality that comes with your YAML tests. I'm open to going back to having them here though if folks don't see the benefit of that. We've just always made stacker_blueprints entirely optional to a good experience with the main stacker project.

@Lowercases
Copy link
Member Author

Gotcha, after some thought these last few days I think it's positive not to make stacker_blueprints mandatory, but optional :) If you think these can be used directly from stacker, let's do it! I'll adapt stacker_blueprints to get it from stacker as well.

@russellballestrini
Copy link
Member

Should we close this in favor of cloudtools/stacker#606?

@Lowercases
Copy link
Member Author

Lowercases commented Jul 25, 2018

@russellballestrini this has been superseded by stacker's merge indeed, however we still could port the tests to yaml -- I'll migrate this PR to do that.

Thanks for bringing this to attention though, it had slipped through the cracks for me!

@phobologic
Copy link
Member

stacker 1.4.0 is out, with the yaml tests, so if you want to update the required version to stacker 1.4.0, it should have everything we need :)

Upgrade stacker requirement to >= 1.4.0
@Lowercases
Copy link
Member Author

@phobologic we're r2g!

@phobologic phobologic merged commit 56ae495 into master Aug 6, 2018
@phobologic phobologic deleted the yaml-tests branch August 6, 2018 17:42
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