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

y-yagi · 2020-11-23T04:52:44Z

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 · 2020-11-23T08:18:00Z

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 · 2020-11-24T05:17:14Z

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

benoittgt · 2020-11-24T08:43:33Z

The feature test pass without your initial patch 😅

y-yagi · 2020-11-24T08:58:27Z

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 · 2020-11-24T09:05:21Z

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 · 2020-11-24T09:07:32Z

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 · 2020-11-26T03:26:15Z

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.).

benoittgt

LGTM @y-yagi

Thanks for the patch.

benoittgt · 2020-11-26T08:23:13Z

lib/rspec/rails/example/system_example_group.rb

+            RSpec.current_example.description.underscore
+          ].join("_").tr(CHARS_TO_TRANSLATE.join, "_")
+
+          if RbConfig::CONFIG["host_os"] =~ /linux/


I thought we had something for that in https://github.com/rspec/rspec-support/blob/12069e1c1ad1852b7970e7fcd419d56a8a9e6240/lib/rspec/support/ruby_features.rb#L9 but this is not the case.

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

JonRowe · 2020-11-27T17:34:16Z

spec/rspec/rails/example/system_example_group_spec.rb

+          end
+
+          example = group.new
+          example_class_mock = double('name' => 'あ'*100)


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

JonRowe · 2020-11-27T17:34:40Z

lib/rspec/rails/example/system_example_group.rb

+            RSpec.current_example.description.underscore
+          ].join("_").tr(CHARS_TO_TRANSLATE.join, "_")
+
+          if RbConfig::CONFIG["host_os"] =~ /linux/


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

`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 · 2020-11-28T04:06:44Z

@JonRowe Thanks for your review. I fixed.

JonRowe · 2021-03-17T08:54:37Z

Merged in ff8eaef

JonRowe · 2021-03-17T08:55:08Z

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

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

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

benoittgt approved these changes Nov 26, 2020

View reviewed changes

JonRowe requested changes Nov 27, 2020

View reviewed changes

y-yagi force-pushed the consider-multibyte-in-system-spec branch from b3c9fad to 49b1090 Compare November 28, 2020 03:57

y-yagi force-pushed the consider-multibyte-in-system-spec branch from 49b1090 to 863a722 Compare November 28, 2020 03:58

benoittgt approved these changes Nov 29, 2020

View reviewed changes

JonRowe mentioned this pull request Mar 17, 2021

Fix method name truncation for multibyte strings (redux) #2487

Merged

JonRowe closed this Mar 17, 2021

y-yagi deleted the consider-multibyte-in-system-spec branch March 17, 2021 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

y-yagi commented Nov 23, 2020

benoittgt commented Nov 23, 2020

y-yagi commented Nov 24, 2020

benoittgt commented Nov 24, 2020

y-yagi commented Nov 24, 2020

y-yagi commented Nov 24, 2020

JonRowe commented Nov 24, 2020

y-yagi commented Nov 26, 2020

benoittgt left a comment

benoittgt Nov 26, 2020

JonRowe Nov 27, 2020

JonRowe Nov 27, 2020

JonRowe Nov 27, 2020

y-yagi commented Nov 28, 2020

JonRowe commented Mar 17, 2021

JonRowe commented Mar 17, 2021

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

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

Conversation

y-yagi commented Nov 23, 2020

benoittgt commented Nov 23, 2020

y-yagi commented Nov 24, 2020

benoittgt commented Nov 24, 2020

y-yagi commented Nov 24, 2020

y-yagi commented Nov 24, 2020

JonRowe commented Nov 24, 2020

y-yagi commented Nov 26, 2020

benoittgt left a comment

Choose a reason for hiding this comment

benoittgt Nov 26, 2020

Choose a reason for hiding this comment

JonRowe Nov 27, 2020

Choose a reason for hiding this comment

JonRowe Nov 27, 2020

Choose a reason for hiding this comment

JonRowe Nov 27, 2020

Choose a reason for hiding this comment

y-yagi commented Nov 28, 2020

JonRowe commented Mar 17, 2021

JonRowe commented Mar 17, 2021