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

[WIP]Write rubocop rspec and test rake task #865

Merged
merged 19 commits into from Jan 5, 2019

Conversation

@faithngetich
Copy link
Collaborator

faithngetich commented Oct 12, 2018

No description provided.

@faithngetich faithngetich changed the title [WIP]Write rubocop rspec and test rakefile [WIP]Write rubocop rspec and test rake task Oct 12, 2018
Copy link
Owner

roschaefer left a comment

@faithngetich thank you so much for your pull request. From the quality of the code I can see that you invested some time to do the research! 👍 🥇 It's awesome.

Feel free to change the .travis.yml. But make sure that the script always fails if any test fails or if some security vulnerabilities are detected.

The following task should be executed for each directory

  • frontend: ember test
  • backend: bin/rake build:brakeman build:rspec
  • root: bin/rake build:rubocop build:cucumber
    The last rubocop task should also check the backend folder.

Furthermore I would like to ask you to pair with @lindseyperry. She is becoming a product manager for open source projects and I think it would be super interesting to show Lindsey what a build server is and why it is so important. This PR is a great example, I think 🙂

backend/lib/tasks/rubocop_brakeman_rspec.rake Outdated Show resolved Hide resolved
backend/lib/tasks/rubocop_brakeman_rspec.rake Outdated Show resolved Hide resolved
options = {
app_path: '.',
exit_on_error: EXIT_ON_FAIL,
exit_on_warn: EXIT_ON_FAIL,

This comment has been minimized.

Copy link
@roschaefer

roschaefer Oct 15, 2018

Owner

Make sure the tasks exits with a non-zero return value if brakeman detects something. This will make our build fail and that's what we want.

You can verify this by removing this file here. If you run bin/rake build_server:brakeman && echo "No fail!" it should not print No fail. If it does, then the echo was reached and that's not what you want.

@faithngetich faithngetich force-pushed the 837_rspec_breakman_rubocop_rake_task branch 3 times, most recently from aaec120 to c8cdeeb Nov 2, 2018
@faithngetich faithngetich force-pushed the 837_rspec_breakman_rubocop_rake_task branch from c8cdeeb to 8251bfe Nov 5, 2018
faithngetich added 2 commits Nov 7, 2018
- to run both rubocop and cucumber as tasks in the build server
@faithngetich

This comment has been minimized.

Copy link
Collaborator Author

faithngetich commented Nov 7, 2018

@roschaefer I have implemented the changes we discussed in the meeting. Kindly review.

roschaefer added 2 commits Nov 14, 2018
Reasoning:
1. We want every error to fail the entire build, so we put every command
on a new line in .travis.yml
2. We want to run all commands as a feedback, so a developer can fix e.g.
rubocop+rspec at once
Copy link
Owner

roschaefer left a comment

@faithngetich I double checked and the rake tasks and they don't exit with a non-zero code. This behaviour would not fail the build! E.g. we would have failing tests but the build server would not warn us and we deploy errors in production! That's not what we want. Please fix the rake tasks 🙏

Rakefile Outdated

task default: [:features]
task rubocop_and_cucumber: %i[rubocop features]

This comment has been minimized.

Copy link
@roschaefer

roschaefer Nov 14, 2018

Owner

Please add a description so that we see this task when bundle exec rake -T

This comment has been minimized.

Copy link
@roschaefer

roschaefer Nov 14, 2018

Owner

Also namespace this task: bundle exec rake build:rubocop_and_cucumber

This comment has been minimized.

Copy link
@roschaefer

roschaefer Nov 14, 2018

Owner

or just bundle exec rake rubocop features and remove this task

task.options << '--display-cop-names'

# Abort on failures (fix your code first)
task.fail_on_error = false

This comment has been minimized.

Copy link
@roschaefer

roschaefer Nov 14, 2018

Owner

@faithngetich why do you set task.fail_on_error = false? 🤔 I tried it on my machine and it does return a non-zero exit code and would not fail the build server if it found some codestyle violations. But that's what we want..

This comment has been minimized.

Copy link
@roschaefer

roschaefer Jan 4, 2019

Owner

@faithngetich and @ciremoussadia can sb. of you double-check if bundle exec rake rubocop exits with a non-zero code in case of failing tests? It's very important not to break our build pipeline. We don't want the rspec tests to fail silently.

https://travis-ci.org/roschaefer/rundfunk-mitbestimmen/builds/475395660#L744

by checking the seed data at the latest
@ciremoussadia

This comment has been minimized.

Copy link
Collaborator

ciremoussadia commented Jan 3, 2019

@faithngetich @roschaefer can I give a try on this one?

@faithngetich

This comment has been minimized.

Copy link
Collaborator Author

faithngetich commented Jan 4, 2019

A file was testing a rake task that has been some commits behind. That caused the test to fail.
@ciremoussadia

This comment has been minimized.

Copy link
Collaborator

ciremoussadia commented Jan 4, 2019

@roschaefer Ready to merge so.

@roschaefer

This comment has been minimized.

Copy link
Owner

roschaefer commented Jan 4, 2019

@ciremoussadia this line here will not silently fail the build? If you double-checked that, I'll hit the merge button.

@roschaefer

This comment has been minimized.

Copy link
Owner

roschaefer commented Jan 4, 2019

@ciremoussadia did you add a security vulnerability and checked if bundle exec rake brakeman fails?

@roschaefer

This comment has been minimized.

Copy link
Owner

roschaefer commented Jan 4, 2019

Sorry for being so pedantic - but we really want to trust our build server 😉

@ciremoussadia

This comment has been minimized.

Copy link
Collaborator

ciremoussadia commented Jan 4, 2019

@roschaefer you right. We better make everything is working as expected. How do I add a security vulnerability? Never does it before.

@roschaefer

This comment has been minimized.

Copy link
Owner

roschaefer commented Jan 4, 2019

@ciremoussadia the brakeman config ignores one particular security warning, which is a false positive. You can simply remove that file and check if bundle exec rake brakeman fails with a non-zero exit code, similar to bundle exec brakeman -zq.

@roschaefer

This comment has been minimized.

Copy link
Owner

roschaefer commented Jan 4, 2019

@ciremoussadia by the way, this technique of breaking sth. deliberately and check if one of the tests is failing is called mutation testing. I do it all the time.

@ciremoussadia

This comment has been minimized.

Copy link
Collaborator

ciremoussadia commented Jan 4, 2019

@roschaefer after removing that file, I got an SQL Injection Warning. Does it means it's working?

@roschaefer

This comment has been minimized.

Copy link
Owner

roschaefer commented Jan 4, 2019

@ciremoussadia sounds good! If bundle exec rake brakeman || echo "Fails as expected" prints "Fails as expected" everything is good. That means, it not only prints the failure to the standard output but it will also fail the build.

@ciremoussadia

This comment has been minimized.

Copy link
Collaborator

ciremoussadia commented Jan 4, 2019

@roschaefer yeah it does when i remove the file. Can u explain me why are we ignoring SQL Injections?

task.fail_on_error = false
@roschaefer

This comment has been minimized.

Copy link
Owner

roschaefer commented Jan 4, 2019

@ciremoussadia the SQL injection warning is a false positive apparently brakeman checks for any interpolated strings that get sent to the database. The reasoning is that interpolated strings may contain data that was entered by the user. That would be an SQL injection. But in our case we're interpolating a string with no user data. No danger here. Therefore a false positive.

@roschaefer

This comment has been minimized.

Copy link
Owner

roschaefer commented Jan 4, 2019

@ciremoussadia did you make a failing rspec tests and bundle exec rake rspec || echo "All good" outputs "All good"?

@ciremoussadia

This comment has been minimized.

Copy link
Collaborator

ciremoussadia commented Jan 4, 2019

@roschaefer everything is working.

@roschaefer roschaefer merged commit 15a8198 into master Jan 5, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@roschaefer roschaefer deleted the 837_rspec_breakman_rubocop_rake_task branch Jan 5, 2019
@roschaefer

This comment has been minimized.

Copy link
Owner

roschaefer commented Jan 5, 2019

@faithngetich and @ciremoussadia thank you so much for your help! Let's have an eye on our build server for the next couple of pull requests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.