Improving test env and travis workflow

[alaxalves]

Fixes #598 and part of #599

• PR is descriptively titled 📑 and links the original issue above 🔗
• tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
• code is in uniquely-named feature branch and has no merge conflicts 📁
• screenshots/GIFs are attached 📎 in case of UI updation
• ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

[codeclimate]

Code Climate has analyzed commit d30e2f48 and detected 0 issues on this pull request.

View more on Code Climate.

[alaxalves]

I could take advantage of this and dockerize the development environment and work on #604, also I'd like to use Travis' build matrix to parallel our test scripts instead of runnig all tests in a single directive. Something like this:

In this case the unit tests have passed but acceptance tests didn't. It makes debugging pretty easy.

What do you think?? @jywarren @icarito @cesswairimu

[codecov]

Codecov Report

Merging #605 into main will decrease coverage by 29.65%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main     #605       +/-   ##
===========================================
- Coverage   71.27%   41.62%   -29.66%     
===========================================
  Files          32       33        +1     
  Lines        1288     1295        +7     
===========================================
- Hits          918      539      -379     
- Misses        370      756      +386

Impacted Files	Coverage Δ
app/controllers/comments_controller.rb	13.79% <0%> (-86.21%)	⬇️
app/controllers/maps_controller.rb	22.41% <0%> (-71.23%)	⬇️
app/controllers/feeds_controller.rb	30% <0%> (-70%)	⬇️
app/controllers/tags_controller.rb	22.72% <0%> (-68.19%)	⬇️
app/models/export.rb	27.77% <0%> (-66.67%)	⬇️
app/helpers/application_helper.rb	15.62% <0%> (-65.63%)	⬇️
app/controllers/images_controller.rb	17.39% <0%> (-57.98%)	⬇️
app/controllers/users_controller.rb	46.66% <0%> (-53.34%)	⬇️
app/mailers/comment_mailer.rb	50% <0%> (-50%)	⬇️
app/controllers/export_controller.rb	20.4% <0%> (-48.98%)	⬇️
... and 9 more

[alaxalves]

@publiclab/reviewers when I run tests with the rake test:all directive, all of the mapknitter's test pass
but when I run rake test:units to run only the unit tests I'm getting the following error:

Build: https://travis-ci.org/alaxalves/mapknitter/jobs/534312520

[cesswairimu]

Hey @alaxalves, I just looked at your build the tests seems to be running differently and smoothly. I don't know much about travis setup to leave an informative review but will rebase this with my branch see if my build finishes. Thanks

[sashadev-sky]

@alaxalves I created these rake tasks a while back in order to hide all the warnings. If you go to test_unit.rake, there are a few different ways to run tests (parallel, one at a time) with a commented description of each. When I made them I noticed that rake test:unit failed because it was taking too long but I think you can try the alternative rake test:unit_parallel.

[alaxalves]

@alaxalves I created these rake tasks a while back in order to hide all the warnings. If you go to test_unit.rake, there are a few different ways to run tests (parallel, one at a time) with a commented description of each. When I made them I noticed that rake test:unit failed because it was taking too long but I think you can try the alternative rake test:unit_parallel.

Don't you think it's better keeping the default task provided by Rake gem? It would be less code to maintain. I don't like the idea of omitting the warnings generated by the test, with that we could debug easier when running a test and we could also keep easier tracking of the outdated rake version and such.

[alaxalves]

@jywarren Could you also take a look at this and give us directions on how to proceed with this problematic test

[jywarren]

Sure, this sounds reasonable to me - what do other people think? On Tue, May 28, 2019 at 4:15 PM Álax de Carvalho Alves < notifications@github.com> wrote:

…

I'm curious - how bad are the warnings now that we've done a lot of upgrading already? Is now the right moment to make this change, or should we wait until after Rails 4.x is merged? … <#m_5092285205458500581_> On Tue, May 28, 2019 at 8:41 AM Álax de Carvalho Alves < *@*.***> wrote: I think we can transition back to the standard test runner as we address the warnings by doing our Rails and dependency upgrades! But glad to discuss and plan out the transition. Thanks! @jywarren <https://github.com/jywarren> https://github.com/jywarren So, in the .travis.yml script stage I replace script: - docker-compose -f docker-compose.test.yml exec web bash -lc "$TASK" with: script: docker-compose exec web bash -l -c "CI=true TRAVIS=true rake test:all" And we go back with the old setup? Or do you want me to close #605 <#605> <#605 <#605>> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#605 <#605>?email_source=notifications&email_token=AAAF6J2Y2IXKFAVPIRWDY5TPXUR43A5CNFSM4HM5S6Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWL7WPY#issuecomment-496499519>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAF6JYMBOQRUJU4TXUBKPLPXUR43ANCNFSM4HM5S6YQ . This PR is about splitting our environments, since we're running the Production environment on Travis, I have described this issue a little more here <#599>. I don't think it's a good idea if we keep hiding our warnings, because that would compromise our code base, and would make easier for us to identify a need to change/upgrade some portion of code. Also, it's a good idea for us to use the default tasks provided by Rake - less code to maintain. Outputting the warnings would totally contribute to our Rails upgrades, since one of our goals is to fix all of the outdated warnings, deprecation warnings and such. Maybe @cesswairimu <https://github.com/cesswairimu> @kaustubh-nair <https://github.com/kaustubh-nair> @sashadev-sky <https://github.com/sashadev-sky> @IgorWilbert <https://github.com/IgorWilbert> @icarito <https://github.com/icarito> could endorse me on this one :laugh: — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#605?email_source=notifications&email_token=AAAF6J7O2MOQH4OB4UMVBE3PXWHD3A5CNFSM4HM5S6Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWNJYOY#issuecomment-496671803>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAF6J7VG2MB2AJ4Z2OJN2LPXWHD3ANCNFSM4HM5S6YQ> .

[alaxalves]

Sure, this sounds reasonable to me - what do other people think? On Tue, May 28, 2019 at 4:15 PM Álax de Carvalho Alves < notifications@github.com> wrote:
…
I'm curious - how bad are the warnings now that we've done a lot of upgrading already? Is now the right moment to make this change, or should we wait until after Rails 4.x is merged? … <#m_5092285205458500581_> On Tue, May 28, 2019 at 8:41 AM Álax de Carvalho Alves < @.***> wrote: I think we can transition back to the standard test runner as we address the warnings by doing our Rails and dependency upgrades! But glad to discuss and plan out the transition. Thanks! @jywarren https://github.com/jywarren https://github.com/jywarren So, in the .travis.yml script stage I replace script: - docker-compose -f docker-compose.test.yml exec web bash -lc "$TASK" with: script: docker-compose exec web bash -l -c "CI=true TRAVIS=true rake test:all" And we go back with the old setup? Or do you want me to close #605 <#605> <#605 <#605>> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#605 <#605>?email_source=notifications&email_token=AAAF6J2Y2IXKFAVPIRWDY5TPXUR43A5CNFSM4HM5S6Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWL7WPY#issuecomment-496499519>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAF6JYMBOQRUJU4TXUBKPLPXUR43ANCNFSM4HM5S6YQ . This PR is about splitting our environments, since we're running the Production environment on Travis, I have described this issue a little more here <#599>. I don't think it's a good idea if we keep hiding our warnings, because that would compromise our code base, and would make easier for us to identify a need to change/upgrade some portion of code. Also, it's a good idea for us to use the default tasks provided by Rake - less code to maintain. Outputting the warnings would totally contribute to our Rails upgrades, since one of our goals is to fix all of the outdated warnings, deprecation warnings and such. Maybe @cesswairimu https://github.com/cesswairimu @kaustubh-nair https://github.com/kaustubh-nair @sashadev-sky https://github.com/sashadev-sky @IgorWilbert https://github.com/IgorWilbert @icarito https://github.com/icarito could endorse me on this one :laugh: — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#605?email_source=notifications&email_token=AAAF6J7O2MOQH4OB4UMVBE3PXWHD3A5CNFSM4HM5S6Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWNJYOY#issuecomment-496671803>, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAF6J7VG2MB2AJ4Z2OJN2LPXWHD3ANCNFSM4HM5S6YQ .

BTW I got to fix our problematic test 🎉 🎉 🎉 🎉 🎉

[sashadev-sky]

@alaxalves @jywarren My thought process was for contributors not working on that it's just a lot of code to have to scroll through each time thats not relevant to their work. For ex, this entire block appears 4 separate times:

The current state of the warnings is the same and they can't be fixed without the major rails upgrades you guys are doing because they come from dependencies, not our code directly.

But theres a reason rails doesn't give us the option to ignore warnings built in and thats because your points are right. The intent behind them, though, was that they were a barrier to entry for newcomers rather than being a great practice. Anyway, i'm all for actually removing them and if this helps with collaboration I say go for it. Hopefully in a few weeks we wont need them anyway 👍Maybe we can switch the default for now to running with warnings and leave /explain the option to run locally without in the README

[alaxalves]

@alaxalves @jywarren My thought process was for contributors not working on that it's just a lot of code to have to scroll through each time thats not relevant to their work. For ex, this entire block appears 4 separate times:

The current state of the warnings is the same and they can't be fixed without the major rails upgrades you guys are doing because they come from dependencies, not our code directly.

But theres a reason rails doesn't give us the option to ignore warnings built in and thats because your points are right. The intent behind them, though, was that they were a barrier to entry for newcomers rather than being a great practice. Anyway, i'm all for actually removing them and if this helps with collaboration I say go for it. Hopefully in a few weeks we wont need them anyway Maybe we can switch the default for now to running with warnings and leave /explain the option to run locally without in the README

Thnx @sashadev-sky. I totally agree with you and I also think that's a great idea to put in our README how to run tests without the warnings, in fact I could do this in this PR you you'd like. What do you think @jywarren @sashadev-sky ?? Maybe we could merge this after.

[cesswairimu]

Just a note here, all these warnings go away after the upgrade. Also @alaxalves is the PR ready? I am really looking forward to having it in main 😄 🎉 Thanks all

[alaxalves]

Just a note here, all these warnings go away after the upgrade. Also @alaxalves is the PR ready? I am really looking forward to having it in main Thanks all

@cesswairimu It's completely ready, I'm just waiting for someone to merge it. 😄

[cesswairimu]

Awesome @gauravano and @jywarren please review this when you get a minute. Thanks

[sashadev-sky]

@cesswairimu @alaxalves 🙌🏻🙌🏻

@alaxalves You can add it here or just submit a patch in a new PR!

[alaxalves]

@cesswairimu @alaxalves 🙌🏻🙌🏻

@alaxalves You can add it here or just submit a patch in a new PR!

It's done! Could you check this out too @sashadev-sky ?

[jywarren]

Great work, great teamwork and great discussion folks. Really tremendous! 🙌 This makes me so happy!!!!

[cesswairimu]

Thanks Jeff. Rebasing my branch to see if travis will finish in a few