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

Consider the multibyte value in the method name of system test #2405

Closed
wants to merge 1 commit into from

Conversation

y-yagi
Copy link
Contributor

@y-yagi y-yagi commented Nov 23, 2020

String#[] returns a value that considers multibyte value. But maximum filename length use bytes. So if applications use multibyte value to a method name, currently check doesn't work expected.

This PR fixes to use String#byteslice instead of String#[]. Also, added String#scrub to avoid generating an invalid byte sequence.

@benoittgt
Copy link
Member

Hello @y-yagi

Thanks for the PR. Do you think that:

So if applications use multibyte value to a method name, currently check doesn't work expected.

Could be expressed in a test?

@y-yagi y-yagi force-pushed the consider-multibyte-in-system-spec branch from fa5a3ed to 9fb6fef Compare November 24, 2020 05:12
@y-yagi
Copy link
Contributor Author

y-yagi commented Nov 24, 2020

Hi, @benoittgt.
Thanks for your feedback. I added a test for the issue.

@benoittgt
Copy link
Member

The feature test pass without your initial patch 😅

@y-yagi
Copy link
Contributor Author

y-yagi commented Nov 24, 2020

Hmmm. The feature test failed in my environment(Linux 5.4.0-54-generic, file system is ext4) without the patch.

$ git diff
diff --git a/lib/rspec/rails/example/system_example_group.rb b/lib/rspec/rails/example/system_example_group.rb
index e0f395bc..59c5606c 100644
--- a/lib/rspec/rails/example/system_example_group.rb
+++ b/lib/rspec/rails/example/system_example_group.rb
@@ -41,7 +41,7 @@ module RSpec
         @method_name ||= [
           self.class.name.underscore,
           RSpec.current_example.description.underscore
-        ].join("_").tr(CHARS_TO_TRANSLATE.join, "_").byteslice(0...200).scrub("") + "_#{rand(1000)}"
+        ].join("_").tr(CHARS_TO_TRANSLATE.join, "_")[0...200] + "_#{rand(1000)}"
       end
 
       # Delegates to `Rails.application`. 


$ bundle exec cucumber features/system_specs/system_specs.feature
*** THIS RUBY IMPLEMENTATION DOESN'T REPORT FILE AND LINE FOR PROCS ***
/home/y-yagi/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/gherkin-2.12.2/lib/gherkin/formatter/filter_formatter.rb:105: warning: constant ::Fixnum is deprecated
Using the default profile...
........^T.......F-

(::) failed steps (::)

expected "\nRandomized with seed 40773\n\nWidget management\n  long test name ああああああああああああああああああああああああああああああああ...5.0.4/lib/puma/server.rb:94: warning: deprecated Object#=~ is called on Proc; it always returns nil" not to string includes: "Errno::ENAMETOOLONG"
Diff:
@@ -1,47 +1,93 @@
-Errno::ENAMETOOLONG
+
+Randomized with seed 40773
+
+Widget management
+  long test name ああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああ (FAILED - 1)
+
+Failures:
+
+  1) Widget management long test name ああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああ
+     Got 1 failure and 1 other error:
+
+     1.1) Failure/Error: expect(page).to have_text("Widget was created.")
+            expected to find text "Widget was created." in "Widget was successfully created.
+Name: My Widget
+Category:
+Instock: false
+Foo:
+Bar:
+Edit | Back"
+          # ./spec/system/widget_system_spec.rb:14:in `block (2 levels) in <top (required)>'
+
+     1.2) Failure/Error: File.open(png_path, 'wb') { |f| f << screenshot_as(:png) }
+
+          Errno::ENAMETOOLONG:
+            File name too long @ rb_sysopen - /home/y-yagi/src/github.com/rspec/rspec-rails/tmp/aruba/tmp/screenshots/failures_r_spec_example_groups_widget_management_long_test_name_ああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああ_178.png
+          # /home/y-yagi/src/github.com/rspec/bundle/ruby/2.7.0/gems/selenium-webdriver-3.142.7/lib/selenium/webdriver/common/driver_extensions/takes_screenshot.rb:40:in `initialize'
+          # /home/y-yagi/src/github.com/rspec/bundle/ruby/2.7.0/gems/selenium-webdriver-3.142.7/lib/selenium/webdriver/common/driver_extensions/takes_screenshot.rb:40:in `open'
+          # /home/y-yagi/src/github.com/rspec/bundle/ruby/2.7.0/gems/selenium-webdriver-3.142.7/lib/selenium/webdriver/common/driver_extensions/takes_screenshot.rb:40:in `save_screenshot'
+          # /home/y-yagi/src/github.com/rspec/bundle/ruby/2.7.0/gems/capybara-3.33.0/lib/capybara/selenium/driver.rb:119:in `save_screenshot'
+          # /home/y-yagi/src/github.com/rspec/bundle/ruby/2.7.0/gems/capybara-3.33.0/lib/capybara/session.rb:726:in `block in save_screenshot'
+          # /home/y-yagi/src/github.com/rspec/bundle/ruby/2.7.0/gems/capybara-3.33.0/lib/capybara/session.rb:726:in `tap'
+          # /home/y-yagi/src/github.com/rspec/bundle/ruby/2.7.0/gems/capybara-3.33.0/lib/capybara/session.rb:726:in `save_screenshot'
+
+Top 1 slowest examples (3.2 seconds, 99.6% of total time):
+  Widget management long test name ああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああ
+    3.2 seconds ./spec/system/widget_system_spec.rb:8
+
+Finished in 3.21 seconds (files took 3.72 seconds to load)
+1 example, 1 failure
+
+Failed examples:
+
+rspec ./spec/system/widget_system_spec.rb:8 # Widget management long test name ああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああああ
+
+Randomized with seed 40773
+
+/home/y-yagi/src/github.com/rspec/bundle/ruby/2.7.0/gems/puma-5.0.4/lib/puma/server.rb:94: warning: deprecated Object#=~ is called on Proc; it always returns nil
 (RSpec::Expectations::ExpectationNotMetError)
features/system_specs/system_specs.feature:134:in `And the output should not contain "Errno::ENAMETOOLONG"'

Failing Scenarios:
cucumber features/system_specs/system_specs.feature:112 # Scenario: System specs can save screenshots if the method name is too long

4 scenarios (1 failed, 3 passed)
17 steps (1 failed, 1 skipped, 15 passed)
0m22.318s

What kind of file system do you use?

@y-yagi
Copy link
Contributor Author

y-yagi commented Nov 24, 2020

Sorry, I missed considering APFS. APFS's maximum filename length is 255 UTF-8 characters. So if you use macOS, maybe feature test pass without my patch.
https://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits

@JonRowe
Copy link
Member

JonRowe commented Nov 24, 2020

Our feature tests are our documentation, this isn't something we want in those, a better place would be in:

spec/rspec/rails/example/system_example_group_spec.rb

@y-yagi y-yagi force-pushed the consider-multibyte-in-system-spec branch 2 times, most recently from 85533a1 to b3c9fad Compare November 26, 2020 03:04
@y-yagi
Copy link
Contributor Author

y-yagi commented Nov 26, 2020

Thanks, JonRowe. I moved tests to spec/rspec/rails/example/system_example_group_spec.rb.

Also, I fixed to check considering OS(It seems that difficult to check file systems by Ruby's native API. So I checked the OS.).

Copy link
Member

@benoittgt benoittgt left a comment

Choose a reason for hiding this comment

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

LGTM @y-yagi

Thanks for the patch.

RSpec.current_example.description.underscore
].join("_").tr(CHARS_TO_TRANSLATE.join, "_")

if RbConfig::CONFIG["host_os"] =~ /linux/
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Don't make it OS specific, it's fine to be shorted on all platforms to satisfy linux.

end

example = group.new
example_class_mock = double('name' => 'あ'*100)
Copy link
Member

Choose a reason for hiding this comment

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

Name is generated from the description class / string, set it to a long generated string instead.

RSpec.current_example.description.underscore
].join("_").tr(CHARS_TO_TRANSLATE.join, "_")

if RbConfig::CONFIG["host_os"] =~ /linux/
Copy link
Member

Choose a reason for hiding this comment

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

Don't make it OS specific, it's fine to be shorted on all platforms to satisfy linux.

@y-yagi y-yagi force-pushed the consider-multibyte-in-system-spec branch from b3c9fad to 49b1090 Compare November 28, 2020 03:57
`String#[]` returns a value that considers multibyte value. But some
file systems use byte for maximum filename length. So if applications
use that file system and multibyte value to a method name, currently
check doesn't work expected.

This PR fixes to use `String#byteslice` instead of `String#[]`. Also,
added `String#scrub` to avoid generating an invalid byte sequence.
@y-yagi y-yagi force-pushed the consider-multibyte-in-system-spec branch from 49b1090 to 863a722 Compare November 28, 2020 03:58
@y-yagi
Copy link
Contributor Author

y-yagi commented Nov 28, 2020

@JonRowe Thanks for your review. I fixed.

@JonRowe
Copy link
Member

JonRowe commented Mar 17, 2021

Merged in ff8eaef

@JonRowe JonRowe closed this Mar 17, 2021
@JonRowe
Copy link
Member

JonRowe commented Mar 17, 2021

Thanks for this @y-yagi I tweaked your spec lightly, but this is now merged into main and will be released as part of 5.0.1 at some point soonish

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