Skip to content

Move build test to its own file and test more aspects of build#354

Closed
marksiemers wants to merge 9 commits into
amberframework:masterfrom
marksiemers:conditional-spec-builds-342-a
Closed

Move build test to its own file and test more aspects of build#354
marksiemers wants to merge 9 commits into
amberframework:masterfrom
marksiemers:conditional-spec-builds-342-a

Conversation

@marksiemers

Copy link
Copy Markdown
Contributor

See #348

A big refactor of the specs was done before merging #348 - It was easier to just redo these changes.

@marksiemers

Copy link
Copy Markdown
Contributor Author

@amberframework/core-team - These tests will still fail. Based on our discussion of CSRF in the generated controller spec files, it sounds like a fix could go in that will make the tests pass.

In order to get them to pass, there is an issue that needs to be resolved in amber-spec though.

It is fixed here: https://github.com/amberframework/amber-spec/pull/7
But that line change (or other fix) could be incorporated into the other changes happening with amber-spec.

@marksiemers

Copy link
Copy Markdown
Contributor Author

Another error has been uncovered. I'm going to comment some lines out, but this will be good to address for a next release (getting this integration test to pass with everything):

 158.     # Cast params and set fields.
 159.     private def cast_to_field(name, value : Type)
 160.       if !value.nil?
 161.         case name.to_s
 162.
 163.           when "user_id"
 164.
>165.               @user_id = value.to_i64
 166.
 167.
 168.           when "post_id"
...
undefined method 'to_i64' for Array(JSON::Type) (compile-time type is (Array(JSON::Type) | Bool | Float64 | Hash(String, JSON::Type) | Int64 | String))

The full trace is here: https://www.zerobin.net/?85f41ce66ae44756#jqdAoilihS2iB+bX7Kdx+poCoa6lIYc17XDKnr3nm+A=

@drujensen

Copy link
Copy Markdown
Member

I think this was fixed here. Need to release granite-orm

@marksiemers

Copy link
Copy Markdown
Contributor Author

It seems this integration test will be good at catching bugs introduced by dependencies and/or breaking changes.

@marksiemers

Copy link
Copy Markdown
Contributor Author

You'll see that I gutted a lot of what is being tested: 21b1083

If issues are resolved (like @drujensen mentioned), then some of these can be added back in.

The thing that needs to be fixed is the amber-spec constant issue. The thing that can be fixed is the with granite and the JSON casting thing.

@drujensen A couple questions:

  1. For the amber-spec - any chance a change will go in to address the same thing as amberframework/amber-spec#7
  2. For the granite fix, that is waiting on a release? Any chance of that happening before the next Amber release?

@drujensen

Copy link
Copy Markdown
Member

@marksiemers yeah, i will release it tonight when i get a chance

@eliasjpr

eliasjpr commented Nov 2, 2017

Copy link
Copy Markdown
Contributor

I can release if that's okay with you @drujensen

@drujensen

Copy link
Copy Markdown
Member

Please do. I’m swamped.

@drujensen

Copy link
Copy Markdown
Member

I released Granite 0.7.6. This should fix the issue with to_i64 against a JSON::Type.

elorest and others added 2 commits November 3, 2017 19:51
* Update dependencies

* Remove branch comment

* Fix shard build

* Use test app name instead of path

* More explicit build command to see failures

* Downgrade mosop/cli

* Try previous version of kilt
@marksiemers

marksiemers commented Nov 4, 2017

Copy link
Copy Markdown
Contributor Author

@amberframework/core-team - It looks like the change to amber-spec may have an issue. The error in this test is undefined method 'get':

Error in line 1: while requiring "./spec/controllers/animal_controller_spec.cr"

in spec/controllers/animal_controller_spec.cr:18: undefined method 'get'

      get "/animals"
      ^~~

@marksiemers

Copy link
Copy Markdown
Contributor Author

If #381 goes in, this can likely be closed, it includes these changes plus more.

@marksiemers marksiemers added this to the 0.4.0 - Features & Bugfixes milestone Nov 11, 2017
@marksiemers

Copy link
Copy Markdown
Contributor Author

Done in #381

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.

5 participants