handle long or duplicated screenshot filenames

[JosieMcClellan]

Fixes #32346

Long screenshot filenames in system tests had been throwing Errno::ENAMETOOLONG. This was mostly a problem with a Linux/RSpec combo because the EXT4 filesystem limits filenames to 255 characters, and RSpec Rails uses the full describe/context/it nesting in the filename. This commit rescues Errno::ENAMETOOLONG and truncates the name to its first 225 characters plus the seconds since epoch.

This commit also handles duplicate filenames, which become more common when truncating. E.g, a long describe/context nesting may have the same first 225 chars for many tests. It was already possible though, especially with multiple manual screenshots:

take_screenshot
click_on(something) # I'm trying to isolate this effect
take_screenshot

With this commit, duplicate filenames have a unique suffix appended instead of overwriting the existing file.

Other changes:

• improve testing slightly
  • address some previously untested behavior
  • make a few tests more isolated
  • break down some lib methods to make them more testable
• remove #absolute_image_path because #image_path had become absolute
• tiny refactors

[dillonwelch]

I don't see this logic anymore, is that intended?

[JosieMcClellan]

I'm new to open source, so I apologize if I overstepped what I should change. It seemed the "simple" value wasn't actually used for anything; only the special values "artifact" and "inline" were used. I'm happy to put "simple" back as the default.

[eileencodes]

That's ok @JosiMcClellan - the "simple" is used if the output type set in the above in the ENV is not set. So if RAILS_SYSTEM_TESTING_SCREENSHOT and CAPYBARA_INLINE_SCREENSHOT are not set it will default to simple.

However you're right - it looks like in the display_image case statement simple is not used. I think that might be a mistake and there should be an else (which would refer to simple).

[eileencodes]

Hey @JosiMcClellan thanks for opening your first PR to Rails. I know you're new so don't take this personally but I think the changes here are a lot broader than they need to be.

Do we know the max length that is allowed by the filename? I think in that case we should auto-truncate the name before we even get a ENAMETOOLONG error.

[eileencodes]

That's ok @JosiMcClellan - the "simple" is used if the output type set in the above in the ENV is not set. So if RAILS_SYSTEM_TESTING_SCREENSHOT and CAPYBARA_INLINE_SCREENSHOT are not set it will default to simple.

However you're right - it looks like in the display_image case statement simple is not used. I think that might be a mistake and there should be an else (which would refer to simple).

[JosieMcClellan]

Thank you for the review. I put back simple and started truncating immediately. Is there anything else covered by "broader than they needed to be" that I should undo? Or, I could go back and really just change the bare minimum to add truncation... which would just be adding [0...225] I think

[eileencodes]

Sorry for being unclear - yea I meant to undo everything and just make the bare minimum changes to limit the filename length.

[JosieMcClellan]

Okay, this one is the bare minimum. Thanks for taking the time to help me learn.

[eileencodes]

Thank you!