Add more build specs and include bugfixes#850
Conversation
|
It would be great if we can move all the templates to recipes, not sure if thats possible, but this will removed the overhead of testing the generated apps and have those tests in one central place. Not sure how feasible is this since I know there are concerns about pulling generators from the recipe repos. |
|
@faustinoaq can we move these tests to the Recipe repo? Even if we duplicate the templates for now. @damianham do you think you can work on making a default recipe for amber. @amberframework/contributors lets also brainstorm how we can move the API generator (if possible) Ideally the CLI can be a shell that only runs command without the template dependencies. ❤️ |
All these test (an more) already exist on recipe repo, see: https://github.com/amberframework/recipes/tree/master/spec @eliasjpr We already discussed recipes on #750 and we concluded we gonna try current recipe implementation (optional), and migrate templates to recipes or shards progressively. Currently, This PR is very useful to cover untested features like crecto and ECR templates for amber projects generated without recipes. |
|
@eliasjpr my current thinking based on the work I did on my Maze test framework is to have a base (default) recipe as a shard in the organisation so it would be at https://github.com/amberframework/base I haven't answered all the questions so it is still a work in progress but I think it is moving in the right direction. Going forward we might need to remove the ORM option from the CLI so that the base recipe is Granite only with other ORMs as different recipes. |
@damianham Yeah, you're right, I think we should try current implementation and then bring new infrastructure changes in next minor versions (with well documented guides 😉 ) |
|
Thanks to this PR I found several issues on models, formatting, etc 👍 |
|
This PR is required by amberframework/recipes#23 |
faustinoaq
changed the title
|
|
||
| include CLIHelper | ||
|
|
||
| macro generate_app(*options) |
There was a problem hiding this comment.
why this has to be a macro?
There was a problem hiding this comment.
@eliasjpr I did this to preserve every spec scope (actually this just avoid boilerplate code 😅 )
There was a problem hiding this comment.
I think this can be done without a macro, I will give it a try. The specs should run independently
There was a problem hiding this comment.
I removed macros 👍
Also the describe are running their specs it independently 🎉
|
|
||
| spec_result = `crystal spec` | ||
|
|
||
| puts spec_result |
There was a problem hiding this comment.
Do we want to output here? why not output on failures only?
|
|
||
| puts spec_result | ||
|
|
||
| it "can be executed" do |
There was a problem hiding this comment.
I like this a lot system("crystal spec").should be_true
| spec_result.should_not contain "Error in line" | ||
| end | ||
|
|
||
| it "has no failures" do |
There was a problem hiding this comment.
we can delete this spec if you add system("crystal spec").should be_true
| spec_result.should contain "Finished in" | ||
| end | ||
|
|
||
| it "has no errors" do |
There was a problem hiding this comment.
we can delete this spec if you add system("crystal spec").should be_true
3 participants
Description of the Change
This PR depends on #826
This PR add more build specs like recipes repo already does:
Alternate Designs
Add more built specs (mysql, sqlite)?
Benefits
This will help us to prevent issues with projects ecr and crecto because this combination is currently untested. We had several issues in the past with crecto/ecr mainly because no build specs on them
Possible Drawbacks
This will check all amber features are working fine (well, except other db 😅 )