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

New travis config and rails4 #635

Closed

Conversation

cesswairimu
Copy link
Collaborator

@cesswairimu cesswairimu commented May 26, 2019

Test if new travis config will finish if added to rails 4 upgrade branch

else
flash[:error] = 'You must be logged in to add tags'
redirect_to '/login?back_to=/maps/' + @map.slug
redirect_to "/login?back_to=/maps/#{@map.slug}"
Copy link

Choose a reason for hiding this comment

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

Possible unprotected redirect

if logged_in?
save_tags(@map)
redirect_to '/maps/' + @map.slug
redirect_to "/maps/#{@map.slug}"
Copy link

Choose a reason for hiding this comment

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

Possible unprotected redirect

@@ -48,6 +45,12 @@ def destroy
else
flash[:error] = 'You do not have permission to delete that comment.'
end
redirect_to "/maps/#{params[:map_id]}"
redirect_to "/maps/#{@comment.map.slug}"
Copy link

Choose a reason for hiding this comment

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

Possible unprotected redirect

@comment.update_attribute(:body, params[:comment][:body])
redirect_to '/maps/' + params[:map_id]
@comment.update_attributes(comment_params)
redirect_to "/maps/#{@comment.map.slug}"
Copy link

Choose a reason for hiding this comment

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

Possible unprotected redirect

map_id: @map.slug,
comment: {
post(:create, comment: {
map_id: @map.id,
Copy link

Choose a reason for hiding this comment

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

Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.

@codeclimate
Copy link

codeclimate bot commented May 26, 2019

Code Climate has analyzed commit be3adbc and detected 51 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2
Style 39
Security 10

Note: there are 6 critical issues.

View more on Code Climate.

@codecov
Copy link

codecov bot commented May 26, 2019

Codecov Report

Merging #635 into main will decrease coverage by 39.91%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #635       +/-   ##
===========================================
- Coverage   71.27%   31.35%   -39.92%     
===========================================
  Files          32       32               
  Lines        1288     1502      +214     
===========================================
- Hits          918      471      -447     
- Misses        370     1031      +661
Impacted Files Coverage Δ
app/models/user.rb 74.19% <ø> (-16.44%) ⬇️
app/models/tag.rb 87.5% <ø> (-12.5%) ⬇️
app/models/node.rb 100% <ø> (ø) ⬆️
app/models/annotation.rb 50% <ø> (-30%) ⬇️
app/models/comment.rb 83.33% <ø> (-16.67%) ⬇️
app/models/way.rb 75% <ø> (-5%) ⬇️
app/models/export.rb 25% <0%> (-69.45%) ⬇️
app/controllers/feeds_controller.rb 0% <0%> (-100%) ⬇️
app/controllers/users_controller.rb 0% <0%> (-100%) ⬇️
app/models/map.rb 36.12% <0%> (-48.59%) ⬇️
... and 29 more

@cesswairimu
Copy link
Collaborator Author

cesswairimu commented May 27, 2019

Hi @alaxalves pulled your branch onto mine and travis ran to completion 🎉 🎉
This is how the output looks like.
Screenshot from 2019-05-27 02-53-58

❤️ ❤️
I can not view the main output as a whole though.
Is there something we can add so that we can view the whole build log and also these awesome sub-logs?
Thanks

@kaustubh-nair
Copy link
Member

I can not view the main output as a whole though.
Is there something we can add so that we can view the whole build log and also these awesome sub-logs?
Thanks

Hey Cess, you can view the logs by clicking on the failing jobs right?
I don't think viewing the whole log together is necessary?
Thanks!

@alaxalves
Copy link
Member

alaxalves commented May 27, 2019

Hi @alaxalves pulled your branch onto mine and travis ran to completion
This is how the output looks like.
Screenshot from 2019-05-27 02-53-58

I can not view the main output as a whole though.
Is there something we can add so that we can view the whole build log and also these awesome sub-logs?
Thanks

That's great news @cesswairimu. I think that a good next move would be merging #605 into #578. After this we could start working on the problematic controller test. Meanwhile @kaustubh-nair will keep his good work by increasing our test coverage. I'll keep working in our DevOps infra(it's almost done) and I'll start getting more in touch with you @cesswairimu to get #578 fully merged. What do you think, is that a good workplan for those next days? @jywarren @sashadev-sky @icarito tell us what you think too.

PS @cesswairimu : I have split the build logs on purpose, since it makes easier to debug which test is failing, makes our build faster..... and so on. Basically I took the rake test:all and splitted it, summarizing our entire test suite is still running, but in parallel now.

@cesswairimu
Copy link
Collaborator Author

@alaxalves yeah sounds like a plan. I think the new travis config should be merged to main though so that others can take advantage of it in the meantime. As for the failing test did you test it on your main to see if it fails for you too? I ran the tests on code envy and it was failing there too. I have been trying to fix it but I cannot find the reason why the file is not being created during export.

@kaustubh-nair
Copy link
Member

kaustubh-nair commented May 27, 2019

That's great news @cesswairimu. I think that a good next move would be merging #605 into #578. After this we could start working on the problematic controller test. Meanwhile @kaustubh-nair will keep his good work by increasing our test coverage. I'll keep working in our DevOps infra(it's almost done) and I'll start getting more in touch with you @cesswairimu to get #578 fully merged. What do you think, is that a good workplan for those next days? @jywarren @sashadev-sky @icarito tell us what you think too.

PS @cesswairimu : I have split the build logs on purpose, since it makes easier to debug which test is failing, makes our build faster..... and so on. Basically I took the rake test:all and splitted it, summarizing our entire test suite is still running, but in parallel now.

I will soon be done with the test coverage. After that @cesswairimu I can give your upgrade some help if you'd like. We should push the upgrade to unstable (Mapknitter has an unstable right?) so everyone can test it and create issues.
A good step after that would be changing Bower to Yarn (#351)

@alaxalves
Copy link
Member

That's great news @cesswairimu. I think that a good next move would be merging #605 into #578. After this we could start working on the problematic controller test. Meanwhile @kaustubh-nair will keep his good work by increasing our test coverage. I'll keep working in our DevOps infra(it's almost done) and I'll start getting more in touch with you @cesswairimu to get #578 fully merged. What do you think, is that a good workplan for those next days? @jywarren @sashadev-sky @icarito tell us what you think too.
PS @cesswairimu : I have split the build logs on purpose, since it makes easier to debug which test is failing, makes our build faster..... and so on. Basically I took the rake test:all and splitted it, summarizing our entire test suite is still running, but in parallel now.

I will soon be done with the test coverage. After that @cesswairimu I can give your upgrade some help if you'd like. We should push the upgrade to unstable (Mapknitter has an unstable right?) so everyone can test it and create issues.
A good step after that would be changing Bower to Yarn (#351)

Totally agree, Bower is something that needs quick replacement

@cesswairimu
Copy link
Collaborator Author

Sounds like a plan thanks @alaxalves and @kaustubh-nair

@alaxalves
Copy link
Member

Hey @cesswairimu, I have just finished #605 maybe you could merge it into this to test it out. 😄

@cesswairimu
Copy link
Collaborator Author

Closing this... the travis changes were merged to main.

@cesswairimu cesswairimu closed this Jun 6, 2019
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