-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Make puma log silence completely #2011
Conversation
Hello @ta1kt0me Thanks a lot for your patch. By any chance do you think you can provide a test for this? I will look at the CI (Puma requir Ruby 2.2+ minimum now). |
Thanks for your comment, I'll try it. |
Honestly I'll be happy to merge this when green (depends on #2013) |
CI is now green. Do you think you can rebase a re-push? I can look at this, this afternoon. 👋 |
@benoittgt I'm sorry for late. I have rebased it. |
Hello It seems your test is broken. What I did instead is:
The test fails on:
So fix is good for me. I don't know how much we invest to fix the test. |
chromedriver isn't available on travis without installing it, if theres a driver that works built in that would work, but I can see the puma output in the output... |
Are we accepting this patch without the test? |
I think we need some form of integration or smoke test here really |
@ta1kt0me do you think you have a look. Otherwise I will look at it. |
@benoittgt @JonRowe I'm sorry for replying so late and thank you for your advice. I found the way which resolves #2011 (comment) 's error, but I can not be sure it's good solution. diff --git a/Rakefile b/Rakefile
index 6bb2e0c4..435e361f 100644
--- a/Rakefile
+++ b/Rakefile
@@ -60,7 +60,7 @@ namespace :generate do
# Rails 4 cannot use a `rails` binstub generated by Bundler
sh "rm -f #{bindir}/rails"
- sh "bundle exec rails new ./tmp/example_app --no-rc --skip-javascript --skip-sprockets --skip-git --skip-test-unit --skip-listen --skip-bundle --template=example_app_generator/generate_app.rb"
+ sh "bundle exec rails new ./tmp/example_app --no-rc --skip-javascript --skip-git --skip-test-unit --skip-listen --skip-bundle --template=example_app_generator/generate_app.rb"
in_example_app do
sh "./travis_retry_bundle_install.sh 2>&1" The above makes all spec in green in my local. Or if you have other better solution, it might be good. |
I think the intent to skip sprocket was to make the app smaller. Do you think you can add a commit with this change to check the CI? |
I've added the change. |
It seems their is an issue with the webdriver.
|
As per my earlier comment:
😂 |
df9b606
to
a587501
Compare
@@ -75,6 +75,10 @@ def app | |||
|
|||
attr_reader :driver | |||
|
|||
if ::Rails.version.to_f == 5.1 |
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.
Could you explain why?
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 first implementation is lack of considering of compatibility. action_dispatch_system_test_case
is available from Rails 5.2 . The change is needed to pass System specs driven by selenium_chrome_headless
spec in Rails 5.1 .
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 was literally checking the method exists, then using it, can you explain the "lack of compatibility" I'm afraid I don't understand how "not calling the method if it doesn't exist" changes anything... Also note this need to work on 5.2 and beyond, so this check seems incorrect.
CI seems to be better. |
a587501
to
3ad30b8
Compare
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.
CI is breaking because of SASS on Ruby 1.9.3.
Also unanswered question https://github.com/rspec/rspec-rails/pull/2011/files#r232443748 😊
@@ -60,7 +60,7 @@ namespace :generate do | |||
|
|||
# Rails 4 cannot use a `rails` binstub generated by Bundler | |||
sh "rm -f #{bindir}/rails" | |||
sh "bundle exec rails new ./tmp/example_app --no-rc --skip-javascript --skip-sprockets --skip-git --skip-test-unit --skip-listen --skip-bundle --template=example_app_generator/generate_app.rb" | |||
sh "bundle exec rails new ./tmp/example_app --no-rc --skip-javascript --skip-git --skip-test-unit --skip-listen --skip-bundle --template=example_app_generator/generate_app.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.
Could you explain why you changed this?
This has a cost. It seems that sprockets adds sass gem. On Ruby 1.9.3 it adds deprecation warning and breaks the CI
Failure/Error:
expect(capture_exec(custom_env, exec_script)).to eq(
"#{::Rails.root}/spec/mailers/previews"
)
expected: "/home/travis/build/rspec/rspec-rails/tmp/example_app/spec/mailers/previews"
got: #<struct CaptureExec io="DEPRECATION WARNING:\nSass 3.5 will no longer support Ruby 1.9.3.\nPlease up...ible.\n\n/home/travis/build/rspec/rspec-rails/tmp/example_app/spec/mailers/previews", exit_status=0>
I revert this line and test on Ruby 1.9.3 without issue. I didn't test on all rails version. But maybe it doesn't need to be committed?
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.
Yep I'd like this reverted too
@ta1kt0me Can you please rebase? CI is more stable lately. |
@ta1kt0me Would you like to wrap this up before we release 4.0? |
I can handle it if needed |
This would further confirm your already quite intense comeback! |
Puma log is shown that puma server starts. Using
action_dispatch_system_test_case
load hook can stop its outputBefore
After