ruby / ruby/spec

Extend the tests in regexp/inspect_spec with over escaping combined with character classes

Open
#987 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Ruby
Stars
622
Forks
402
Avg merge
14h 51m
Merged PRs (30d)
6

Description

The specs have one test for escaped backslashes in Regexp#inspect:

it "does not over escape" do
  Regexp.new('\\\/').inspect.should == "/\\\\\\//"
end

The obvious implementation to make this spec pass is to simply escape every slash with another slash. This works in this case, but that causes an error in language/case_spec for something that is not related to the case statement:

it "tests with a regexp interpolated within another regexp" do
  digits_regexp = /\d+/
  case "foo43"
  when /oo(#{digits_regexp})/

This results in a regexp /oo(?-mix:\\d+)/, which now expects a slash followed by one or more d characters. This does not match the input, but does not indicate the escaping as the issue.

I tested my implementation with the these specs added, but this was more or less brute forcing every possibility. Also, because Regexp.new takes a String which has an additional layer of escaping, I peferred the // notation.

Regexp.new('\d+').inspect.should == "/\\d+/"
/\d+/.inspect.should == "/\\d+/"

Regexp.new('\\d+').inspect.should == "/\\d+/"
/\\d+/.inspect.should == "/\\\\d+/"

Regexp.new('\\\d+').inspect.should == "/\\\\d+/"
/\\\d+/.inspect.should == "/\\\\\\d+/"

Regexp.new('\\\\d+').inspect.should == "/\\\\d+/"
/\\\\d+/.inspect.should == "/\\\\\\\\d+/"

I think it would be better to at least include the following cases:

/\d+/ # Output should have 1 not escaped slash, since this should not be escaped
/\\d+/ # Output should have 1 escaped slash (so 2 slashes), this should be escaped
/\\\d+/ # Output should have 1 escaped and 1 not escaped slash (so 3 slashes)

Furthermore, I think it would be better to add an explicit test for the regexp embedded in regexp scenario like there is in the case spec now, just to make the test a bit more explicit and decouple it from the case statement.

I'm not that familiar with this project and how things are structured, can I just add a few tests to the inspect spec of regexp, and make a new spec file for the embedding tests?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with regexp/inspect_spec and the existing escaped-backslash example, then review language/case_spec for the interpolated-regexp behavior. Add coverage for the listed backslash and character-class cases and an explicit embedded-regexp example; the relevant specs should pass without changing the case-statement behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby
Domain
testing
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.