-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Spec Speedup: Use Knapsack to Run Parallel Builds #8390
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
Conversation
1a8aaee
to
8d44430
Compare
8d44430
to
6e8981f
Compare
before_script: | ||
- bundle exec rails db:create | ||
script: | ||
- bundle exec rails webpacker:compile | ||
- bundle exec rails db:schema:load | ||
- './cc-test-reporter before-build' | ||
- 'npx percy exec -- bundle exec rspec spec --color --tty --format progress' | ||
- 'bundle exec rake knapsack_pro:rspec' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible to setup Percy with KnapsackPro but I choose to forego that since we are not using Percy at the moment and wanted to keep this all as simple as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, does using knapsack mean that we'll get 4 csv files sent up to s3 (3 for each knapsack run and 1 for the rss spec run) on master? (thats perfectly fine, just curious).
- 'bundle exec bundle-audit check --update --ignore CVE-2015-9284' | ||
- yarn build-storybook | ||
- bundle exec rails runner -e production 'puts "App booted successfully"' | ||
- if [ "$KNAPSACK_PRO_CI_NODE_INDEX" == "0" ]; then yarn test --colors; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split up other tasks to run on individual nodes
@@ -33,6 +33,14 @@ env: | |||
- SECRET_KEY_BASE=dummydummydummy | |||
- GITHUB_KEY=dummy | |||
- GITHUB_SECRET=dummy | |||
- KNAPSACK_PRO_FIXED_QUEUE_SPLIT=true | |||
- KNAPSACK_PRO_LOG_LEVEL=info | |||
- KNAPSACK_PRO_TEST_FILE_EXCLUDE_PATTERN="spec/services/rss_reader_spec.rb" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RSS reader spec for some reason had a tendency to cause jobs to hang. I think it has to do with how we use the VCR so I choose to remove it from the KnapsackPro specs and run it by itself for now so we could get this out. I plan on looping back to figure out exactly what the issue is there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big 👍 on not letting a single spec file block us!
@@ -442,8 +442,8 @@ | |||
end | |||
|
|||
describe "POST /api/articles" do | |||
let_it_be(:api_secret) { create(:api_secret) } | |||
let_it_be(:user) { api_secret.user } | |||
let(:api_secret) { create(:api_secret) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These specs popped up as flaky when I started running KnapsackPro builds bc of data issues so I changed to using non-persistent objects. I think we should get in the habit of this so eventually we can remove our dependency on order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed! let it be causes trouble
@@ -1,2 +1,3 @@ | |||
--color | |||
--format progress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this flag here so we still get the progress with KnapsackPro
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the order specs are run in isn't fixed, might be worth considering using --format documentation
, so one could see the order at a glance (and debug issues if order happens to be a factor in flakiness). Thats my 2 cents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
knapsack_pro in Regular Mode shows at the beginning of output something like
rspec --default-path spec "spec/system/articles/user_visits_articles_by_timeframe_spec.rb" ...
You can copy-paste to run in the same order tests.
In Queue Mode there will be more examples in output for each given set of tests fetched from Queue. Thanks to that you can run tests in development in the same order.
If you use RSpec seed then you can pass additionally --seed to retry in exact order https://knapsackpro.com/faq/question/how-to-find-seed-in-rspec-output-when-i-use-queue-mode-for-rspec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, I see it now, thanks! no --format documentation
needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one day I think it'd be nice to use --order random
but I don't think we're quite there yet/that could probably be a separate piece of work to get working
.travis.yml
Outdated
@@ -79,7 +86,7 @@ deploy: | |||
condition: $TRAVIS_COMMIT_MESSAGE =~ \[deploy\] | |||
notifications: | |||
slack: | |||
if: branch = master | |||
if: branch = master && "$KNAPSACK_PRO_CI_NODE_INDEX" == "0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only want a single deploy to happen and slack notification too I guess lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized this is for the slack notification, not the actual deploy 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I don't know much about the setup but the build passed and I AM VERY EXCITED FOR THIS!!!
Oh just noticed our CodeClimate coverage went down by a ton. Wonder if that's configurable? |
|
.travis.yml
Outdated
- if [ "$KNAPSACK_PRO_CI_NODE_INDEX" == "2" ]; then bundle exec rspec spec/services/rss_reader_spec.rb; fi | ||
after_script: | ||
- ./cc-test-reporter format-coverage -t simplecov -o ./coverage/codeclimate.$KNAPSACK_PRO_CI_NODE_INDEX.json ./coverage/spec/.resultset.json | ||
- ./cc-test-reporter sum-coverage --output - --parts $KNAPSACK_PRO_CI_NODE_INDEX coverage/codeclimate.*.json | ./cc-test-reporter upload-coverage --input - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the standalone -
do here?
Based on the coverage result on the pr check and on codeclimate, I don't think the sum-coverage here is properly combining all the results. Definitely not a blocker though, we can figure this out afterward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change isn't sending lcov's result
missing for the first node
'./cc-test-reporter format-coverage -t lcov -o coverage/codeclimate.lcov.json'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This I actually stole from @citizen428's parallel travis PR #6645 and the purpose of it is to track the coverage for all the nodes then combine them at the end. However, the code coverage does make it seem like that is not the case... 🤔 Given the % I feel like we got 2 nodes and not 3
Definitely need the total nodes not the node index when combining them 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may find some tips about CodeClimate setup here
https://knapsackpro.com/faq/question/how-to-use-codeclimate-with-knapsack_pro
there are 2 articles but for different CI providers than Travis. But still, you may find some inspiration there.
If you will need at some point in the future setup for simplecov and knapsack_pro Queue Mode then please check this https://knapsackpro.com/faq/question/how-to-use-simplecov-in-queue-mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the standalone - do here?
@maestromac It's a shell thing for representing standard input or output, see e.g. https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
Guideline 13:
For utilities that use operands to represent files to be opened for either reading or writing, the '-' operand should be used to mean only standard input (or standard output when it is clear from context that an output file is being specified) or a file named -.
@@ -33,6 +33,14 @@ env: | |||
- SECRET_KEY_BASE=dummydummydummy | |||
- GITHUB_KEY=dummy | |||
- GITHUB_SECRET=dummy | |||
- KNAPSACK_PRO_FIXED_QUEUE_SPLIT=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KNAPSACK_PRO_FIXED_QUEUE_SPLIT=true
flag is for knapsack_pro Queue Mode. But let's keep it here so you won't forget to add it when you will try Queue Mode.
Here is more explanation of why it's needed for Travis CI
https://github.com/KnapsackPro/knapsack_pro-ruby#knapsack_pro_fixed_queue_split-remember-queue-split-on-retry-ci-node
Basically allows us to remember a split of tests and when you retry only a single failed parallel job then a proper set of tests will be run on retried Travis job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, great to know!!! Yeah I figured I would keep it in for now since queue mode is our goal
Hi all, if you would like to learn more about difference between knapsack_pro Regular Mode and Queue Mode, here you find an article and videos about it @mstruve You can also add a badge to the readme file for a knapsack pro. You can find it next to API token in https://knapsackpro.com/dashboard There is also option to make the Knapsack Pro dashboard public for given API token, just edit API token and enable public dashboard. So when someone clicks on badge then can access dashboard with recorded tests. I added this option a few months ago for open source projects so anyone can access recorded tests. |
- ./cc-test-reporter sum-coverage --output - --parts $COVERAGE_REPORTS_TOTAL coverage/codeclimate.*.json | ./cc-test-reporter upload-coverage --input - | ||
jobs: | ||
include: | ||
- stage: Deploy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensures that all specs pass before we kick off a job to deploy the code. Reference: https://docs.travis-ci.com/user/build-stages/deploy-heroku/
The problem with this right now is that it runs the install steps still, going to work on moving those into the script step for tests.
@@ -33,6 +33,14 @@ env: | |||
- SECRET_KEY_BASE=dummydummydummy | |||
- GITHUB_KEY=dummy | |||
- GITHUB_SECRET=dummy | |||
- KNAPSACK_PRO_FIXED_QUEUE_SPLIT=true | |||
- KNAPSACK_PRO_LOG_LEVEL=info | |||
- KNAPSACK_PRO_TEST_FILE_EXCLUDE_PATTERN="spec/services/rss_reader_spec.rb" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big 👍 on not letting a single spec file block us!
on_success: change | ||
on_failure: always | ||
rooms: | ||
secure: vzIee4jDgPSRY4szZPdD/jW7YW4GzGqo5NoLV9Exz9TBoWH9UqJnc0TOb2YN84Ys5baRK7LOqxpfp8kFveZkrKGi7/ypeEJlpc9E5UqVh/bwQhvOGrKEg1fvNXbARRnO/sJ49o1CMvroMWvt3GurzuuY9Qu2r+3NBjn9aVwLnLzXsBuF+m2lLoeSkHnW13OC73EeJMsse6JBoCe3gp/srDwISp9+MU+sEAPaY333WK9Vk1kdG7D5oUIuT7743airLRiyWiNUCD1450g864628CEOEZKJAAtqk6kTmvwB91DJMnhD/XhMm4H21kd54YHy0fhqzcG8hYd1lDZuUfrOBfpdEtfnpcRwMyMpY+FPPHXkHhck3OiLJnzkV4L+Lr5W/RvDJ63Ye2nxT4hOItLWaoZWax/LhoIrhZjgYBc4JhiGRQJ8m2HzoRyceeG9Y80vayGVN7y46sjYHP5NHRI36qmJipneDRAJklBTXLdYATvVM/6Mh9B7+H/nBGR6UVJLBC/txi2C8rZRjKBZ/i9e+q/MZs0UEvOuvbz9BXKU08rI+rarJqH3h5Ji9G/k3M0mQ8EfvadabA9lu+gNUAAnq+vwLETweKvfbRpDQjVBKnWsOJoUl9aarfkBn3lhQE8fxZJT/GchLGZPx/CWUE4o1OhliBA9avJ7WINyYStM4Mc= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how sensitive this is, just double checking if it might make sense to put this into a Travis env var and read it from there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an encrypted var generated based on their official instruction. It shouldn't be a problem but I can extract it on a different PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maestromac If you don't think it's problematic, save yourself the work. I was just checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Just one question: are the JS tests run three times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 👏 👏 👏 this is going to save all of us so much time!
@rhymes nope! Thanks to a few if statements here https://github.com/thepracticaldev/dev.to/pull/8390/files#diff-354f30a63fb0907d4ad57269548329e3R70 it will only run on node 0 while other tasks will run on the other nodes.
|
LGTM! @mstruve :D |
* Fix erblint errors on the `app/views/listings` folder (#8374) * Fix erblint errors on the `app/views/listings` folder * Move `to_json()` methods to the controller * Update UserDecorator not to cache AR Object (#8378) * Update UserDecorator not to cache AR Object * Change cache name * Change cache name to reduce invalidation frequency * Add json template to Pages (#8357) * Add json template to pages * Add jsonb field, tweak form * Spec Cleanup: Remove Unused and Redundant Code from Specs (#8385) * [deploy] Add crayons to reserved words (#8386) * Fix erblint errors on the `app/views/internal` folder (#8387) * Fix a11y issues flagged by axe on homepage (#8380) * Fix a11y issues flagged by axe on homepage This commit brings the number of violations found on homepage (local, not prod) down from ~100 to ~10. It should have no serious impact on the visual layout or design of the page. * Change aria-labels to be consistent * Flaky Spec Fix: Clear Elasticsearch Data For js: true Specs to Fix Server Errors (#8388) Co-authored-by: Vaidehi Joshi <vaidehi.sj@gmail.com> * [deploy] Bump @percy/agent from 0.26.8 to 0.26.9 (#8325) Bumps [@percy/agent](https://github.com/percy/percy-agent) from 0.26.8 to 0.26.9. - [Release notes](https://github.com/percy/percy-agent/releases) - [Changelog](https://github.com/percy/percy-agent/blob/master/CHANGELOG.md) - [Commits](percy/percy-agent@v0.26.8...v0.26.9) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> * [deploy] Refactor 🚀 : Replaced Chat Channel Setting page with Preact component. (#8271) * Feature 🚀 : Ability to delete messages in chat channels - Sending message ID to frontend - Deleting Message - Use pusher to delete message realtime * Minor Bug 🐞: Show message action only for current user - User can delete or edit their own messages * Test cases added * Bug 🐞: Update message id for receiver Message id was not sent to receiver by pusher * Refactoring🛠: Message controller refactoring * Test Cases📝 : Specs for Delete message added * Feature 🚀 : Ability to edit messages * Test Cases📝 : Specs for Edit message added * 🐞 Channel List bug in mobile view * changes in joining request responses * 🛠 Request Managers Frontend ready * changes in routes for adding and removing remembership * 🚀 New routes implemented for request manager * fix issues * changes in api for remove membership * Merge conflict resolved * 🐞 Channel List bug in mobile view * changes in joining request responses * 🛠 Request Managers Frontend ready * changes in routes for adding and removing remembership * 🚀 New routes implemented for request manager * fix issues * changes in api for remove membership * 🛠Optimizing for CodeClimate * 🛠Optimizing again for CodeClimate * 🛠Optimizing again 2 for CodeClimate * 🛠 Added more test cases * 🛠 Optimizing code * 🚀 Action to open setting page added * 🛠 Dummy page added for chat setting * add JSON routes for chat channel settings * Integrate chat channel settings API with UI * 🐞 Channel List bug in mobile view * 🛠 Request Managers Frontend ready * changes in routes for adding and removing remembership * 🚀 New routes implemented for request manager * fix issues * 🐞 Channel List bug in mobile view * 🛠 Request Managers Frontend ready * changes in routes for adding and removing remembership * 🚀 New routes implemented for request manager * fix issues * 🛠Optimizing again for CodeClimate * 🚀 Action to open setting page added * 🛠 Dummy page added for chat setting * add JSON routes for chat channel settings * Integrate chat channel settings API with UI * Fix PR requested changes * Add JSDoc documentation to exported functions * fix/add rspec test cases * refactor channelSettinngs render part * add test cases for chat channel settings component * fix code-climate * add rest component tests * add more test coverage * fix code-climate bugs * add API function test Co-authored-by: Sarthak Sharma <7lovesharma7@gmail.com> Co-authored-by: Parasgr-code <paras.gaur@skynox.tech> * Fix erblint errors on the `app/views/users` folder (#8389) * Fix erblint errors on the `app/views/users` folder * Fix mistake * Properly return json (#8395) [deploy] * Implement chronological sorting of search results (#8193) * Implement search ordering by allowing sort_by and sort_direction params to be passed through from the search form/page. * Change ID of select to 'sort' instead of 'order' for consistency * Fix styling and data attributes of select options * Enable front end to pass sort_by and sort_direction params for searches to the back end * Ensure that only query string params that are supposed to be changed are changed in each function * Respond to review comments (additional commits to come but these were the quick changes) * Move options for select into template * Use addEventListener instead of using onchange in response to review comments * Add tabs for sorting search results * Finish implementing tabs for ordering search results * Remove CSS rule applying to element that no longer exists * Remove more rules created for an element that has been removed * Remove more code added for the select dropdown that I've removed * Remove duplicate jobs banner * Update example code (#8397) * Fix erblint errors on the `app/views/pages` folder (#8401) * Fix erblint errors on the `app/views/layouts` folder (#8400) * Allow users to dismiss announcements (#8396) [deploy] * Provide default crayon styles when creating banners * Import crayons styles into /internal pages. * Allow admins to use crayons banner styles when creating broadcasts. * Preview banner styles with crayons. * Add a "x" close button to announcements, fix styling to accommodate it * Use constant and helper for broadcast banner styles * Add VALID_BANNER_STYLES frozen constant to Broadcast class. * Add banner_style helper for determining banner style class. * Clean up preview CSS, import broadcast styles into preview * Add close button click handler, clean up initializeBroadcast * Add close functionality to announcement banner Also hide announcement if the user has explicitly seen and "X"-ed out of it. * Add system specs around rendering + dismissing broadcasts * Add some (truly beautiful) JS documentation * Add visible class to broadcast when previewed from internal * [deploy] Spec Speedup: Use Knapsack to Run Parallel Builds (#8390) * Fix erblint errors on the `app/views/doorkeeper` folder (#8399) * Fix erblint errors on the `app/views/mailers` folder (#8402) * Allow Knapsack to work for Forks (#8413) * Add QA instructions to pull request template (#8412) * Bump top-bar's z-index to allow dropdown to render with announcement (#8415) * Fix remaining erblint errors (#8405) * Fix remaining erblint errors * Fix last error * Fix erblint errors on the `app/views/liquids` folder (#8403) * Fix erblint errors on the `app/views/liquids` folder * Make tests pass * Make failing test pass * [deploy] Update GithubReposController to clean up inaccessible repositories (#8382) * Update Users::Sidebar not to cache AR object (#8394) * [deploy] Update ReactionsController not to cache AR Object (#8393) * Hide site-wide messaging for opted-out users (#8409) [deploy] Co-authored-by: Alberto Pérez de Rada Fiol <apdrf.94@gmail.com> Co-authored-by: Mac Siri <krairit.siri@gmail.com> Co-authored-by: Josh Puetz <josh@dev.to> Co-authored-by: Molly Struve <mollylbs@gmail.com> Co-authored-by: Andy Zhao <17884966+Zhao-Andy@users.noreply.github.com> Co-authored-by: Jacob Herrington <jacobherringtondeveloper@gmail.com> Co-authored-by: Vaidehi Joshi <vaidehi.sj@gmail.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: narender2031 <narender2031@gmail.com> Co-authored-by: Sarthak Sharma <7lovesharma7@gmail.com> Co-authored-by: Parasgr-code <paras.gaur@skynox.tech> Co-authored-by: Dana Scheider <dana.scheider@gmail.com> Co-authored-by: Michael Kohl <citizen428@gmail.com>
…orem#8390)" This reverts commit f647944.
What type of PR is this? (check all applicable)
Description
This PR introduces a service called Knapsack to help us parallelize our spec suite as evenly as possible. The first time I ran Knapsack in our build it ran every test separately and recorded the time it took to run the test. Using this information Knapsack then splits the tests up for us into 3 equally timed groups to run in parallel each time we run our test suite. This is how regular mode works.
The changes in this PR are introducing the gem and setting it up. All of them were made with the help of the Knapsack installation guide which walks you through all the changes you should make to get it working properly.
NOTE - There is currently a bug with the parallelization in Travis that causes the
--local
flag for our bundler command to be ignored. This means on your first build, since there is no travis cache, the jobs will likely take 13min. I am in contact with Travis support to get this resolved.Why aren't we using Queue Mode? Ideally, we want to use queue mode. In Queue Mode Knapsack sends us groups of 3-5 specs at once and then when they finish, sends another group of specs. It keeps doing this until all specs have been run. This is obviously the fastest approach but we ran into some errors with the jobs hanging. My goal is to get the regular version out then try to debug that hanging issue so we can use queue mode.
How much does Knapsack cost? FREE bc we are open source and I must say the founder has been extremely helpful in getting us going and holding my hand through the integration process.
@snackattas