Uploaded image for project: 'Maven Surefire'
  1. Maven Surefire
  2. SUREFIRE-1250

Regex testcase filtering: exception when hashmark is regex-quoted

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.19.1
    • Fix Version/s: 2.20
    • Component/s: Maven Surefire Plugin
    • Labels:
      None

      Description

      i've been using regex to select which tests to run...and i've got some wierd exceptions when executing

      mvn test "-Dtest=%regex[.*\Q#\E.*]"
      

      executing the above command results in:

      java.util.regex.PatternSyntaxException: 
      Illegal/unsupported escape sequence near index 1
      \E.*
       ^
      	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:364)
      	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:274)
      	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:238)
      	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:161)
      
      

      i noticed earlier that maven might do some black-magic with these regexps in the background ( i know that it should know the class name earlier; to avoid starting testcases which will get entirely excluded...and thats cool )

      ...so..i assume maven does split the regex by the #; and that's okay - if that can't be avoided; place some pointers in the documentation about it and/or a more descriptive exception could be useful

      1. SUREFIRE-1250.patch
        3 kB
        Zoltan Haindrich

        Issue Links

          Activity

          Hide
          tibor17 Tibor Digana added a comment -

          Zoltan Haindrich
          Did you read this documentation?
          http://maven.apache.org/surefire/maven-surefire-plugin/examples/single-test.html

          The hash mark is class#method separator.
          Then the quotation mark splits in two where.
          Why you use quotations if class name and method name do not use the quotation marks?

          Show
          tibor17 Tibor Digana added a comment - Zoltan Haindrich Did you read this documentation? http://maven.apache.org/surefire/maven-surefire-plugin/examples/single-test.html The hash mark is class#method separator. Then the quotation mark splits in two where. Why you use quotations if class name and method name do not use the quotation marks?
          Hide
          kgyrtkirk Zoltan Haindrich added a comment -

          Tibor Digana

          yes ...i've read the documentation; i don't say it was crystal clear..but i've read it.

          i've opened the source and looked into the code...this exception is caused by the unwrapping of the pattern

          https://github.com/apache/maven-surefire/blob/master/surefire-api/src/main/java/org/apache/maven/surefire/testset/TestListResolver.java#L407

          i've other example regexps which may be affected by the issue:

          • (A#test1|B#test2)
          • A\Q#\Etest

          others may be exists..but if someone hits this thing...they don't have a clue why the regexp is bad - while it's perfectly correct

          so I think a better exception should notify the user about the special nature of these regexps....because i've already downloaded the code; i've added a sample fix for the issue

          Show
          kgyrtkirk Zoltan Haindrich added a comment - Tibor Digana yes ...i've read the documentation; i don't say it was crystal clear..but i've read it. i've opened the source and looked into the code...this exception is caused by the unwrapping of the pattern https://github.com/apache/maven-surefire/blob/master/surefire-api/src/main/java/org/apache/maven/surefire/testset/TestListResolver.java#L407 i've other example regexps which may be affected by the issue: (A#test1|B#test2) A\Q#\Etest others may be exists..but if someone hits this thing...they don't have a clue why the regexp is bad - while it's perfectly correct so I think a better exception should notify the user about the special nature of these regexps....because i've already downloaded the code; i've added a sample fix for the issue
          Hide
          kgyrtkirk Zoltan Haindrich added a comment -

          i think regexps like: A#test1|B#test2 will not cause any errors/warnings; but will behave silently differently than expected by the user.

          Show
          kgyrtkirk Zoltan Haindrich added a comment - i think regexps like: A#test1|B#test2 will not cause any errors/warnings; but will behave silently differently than expected by the user.
          Hide
          tibor17 Tibor Digana added a comment -

          Please concentrate on the problem.
          The symbol "\Q" does not make sense because it's a quotation mark and it cannot not appear in class name.
          Why we are even talking about it?
          I guess this problem is not a problem.

          Show
          tibor17 Tibor Digana added a comment - Please concentrate on the problem. The symbol "\Q" does not make sense because it's a quotation mark and it cannot not appear in class name. Why we are even talking about it? I guess this problem is not a problem.
          Hide
          tibor17 Tibor Digana added a comment -

          Zoltan Haindrich
          The documentation says "The regex validates fully qualified class file, and validates test methods separately after (#)".
          This means the regex argument A#test1|B#test2 is invalid because no such method named "B#test2" can be found in class A. Therefore in this case it has to be comma separated values with two regex, one with A#test1 and another with B#test2.

          Show
          tibor17 Tibor Digana added a comment - Zoltan Haindrich The documentation says "The regex validates fully qualified class file, and validates test methods separately after (#)". This means the regex argument A#test1|B#test2 is invalid because no such method named "B#test2" can be found in class A. Therefore in this case it has to be comma separated values with two regex, one with A#test1 and another with B#test2.
          Hide
          kgyrtkirk Zoltan Haindrich added a comment - - edited

          Tibor Digana
          well...yes... that really makes it clear..but I do read the paragraph containing that phrase multiple times when i was experiencing this exception; maybe an example would have made it more easier for me to comprehend...

          i still think that a custom exception should notify the user in case the regexp usage rules are violated...; maybe even in the case when there are 2 or more #'s.

          if you still think this is not a problem (because it affects only a small portion of users)...then just close this issue as invalid..maybe i'm the only one having issues with it

          Show
          kgyrtkirk Zoltan Haindrich added a comment - - edited Tibor Digana well...yes... that really makes it clear..but I do read the paragraph containing that phrase multiple times when i was experiencing this exception; maybe an example would have made it more easier for me to comprehend... i still think that a custom exception should notify the user in case the regexp usage rules are violated...; maybe even in the case when there are 2 or more # 's. if you still think this is not a problem (because it affects only a small portion of users)...then just close this issue as invalid..maybe i'm the only one having issues with it
          Hide
          tibor17 Tibor Digana added a comment -

          Zoltan Haindrich
          Giving a hint to an user is good point.
          Feel free to commit verification of the pattern and open a pull request on GitHub.
          Throwing exception would be useful.

          Show
          tibor17 Tibor Digana added a comment - Zoltan Haindrich Giving a hint to an user is good point. Feel free to commit verification of the pattern and open a pull request on GitHub. Throwing exception would be useful.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user kgyrtkirk opened a pull request:

          https://github.com/apache/maven-surefire/pull/113

          SUREFIRE-1250: add exceptions to describe usage rules of %regex

          @Tibor17 i've cleaned it up a bit; and added exception for multiple hashmarks too. tests have passed on my machine.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/kgyrtkirk/maven-surefire SUREFIRE-1250

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/maven-surefire/pull/113.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #113


          commit 09e33b88e82266a3118b9db260496f30a8861e58
          Author: Haindrich Zoltán (kirk) <kirk@rxd.hu>
          Date: 2016-05-24T20:28:11Z

          SUREFIRE-1250: add exceptions to describe usage rules of %regex


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user kgyrtkirk opened a pull request: https://github.com/apache/maven-surefire/pull/113 SUREFIRE-1250 : add exceptions to describe usage rules of %regex @Tibor17 i've cleaned it up a bit; and added exception for multiple hashmarks too. tests have passed on my machine. You can merge this pull request into a Git repository by running: $ git pull https://github.com/kgyrtkirk/maven-surefire SUREFIRE-1250 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/maven-surefire/pull/113.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #113 commit 09e33b88e82266a3118b9db260496f30a8861e58 Author: Haindrich Zoltán (kirk) <kirk@rxd.hu> Date: 2016-05-24T20:28:11Z SUREFIRE-1250 : add exceptions to describe usage rules of %regex
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Tibor17 commented on a diff in the pull request:

          https://github.com/apache/maven-surefire/pull/113#discussion_r64666590

          — Diff: surefire-api/src/main/java/org/apache/maven/surefire/testset/TestListResolver.java —
          @@ -479,6 +481,27 @@ else if ( hasMethod )
          updatedFilters( isExcluded, test, patterns, includedFilters, excludedFilters );
          }
          }
          +
          + private static void regexSanityCheck( String request, String classNameExpr, String methodNameExpr )
          + {
          + // this will emit any exception if the regex is not valid at all
          + Pattern.compile( request );
          + try
          + {
          + Pattern.compile( classNameExpr );
          + Pattern.compile( methodNameExpr );
          + if ( methodNameExpr.indexOf( "#" ) != -1 )
          — End diff –

          I thought you would use `String.contains` in both constructors of `ResolvedTest`.
          I think we do not need to use `Pattern.compile` here because this is already done by `maven-shared-utils` anyway.
          Lets use the sanity check if the `isRegex` is set, because I do not know how Cucumber would use JUnit Description of method or class name without regex. Cucumber is different runner from most of the other.

          Show
          githubbot ASF GitHub Bot added a comment - Github user Tibor17 commented on a diff in the pull request: https://github.com/apache/maven-surefire/pull/113#discussion_r64666590 — Diff: surefire-api/src/main/java/org/apache/maven/surefire/testset/TestListResolver.java — @@ -479,6 +481,27 @@ else if ( hasMethod ) updatedFilters( isExcluded, test, patterns, includedFilters, excludedFilters ); } } + + private static void regexSanityCheck( String request, String classNameExpr, String methodNameExpr ) + { + // this will emit any exception if the regex is not valid at all + Pattern.compile( request ); + try + { + Pattern.compile( classNameExpr ); + Pattern.compile( methodNameExpr ); + if ( methodNameExpr.indexOf( "#" ) != -1 ) — End diff – I thought you would use `String.contains` in both constructors of `ResolvedTest`. I think we do not need to use `Pattern.compile` here because this is already done by `maven-shared-utils` anyway. Lets use the sanity check if the `isRegex` is set, because I do not know how Cucumber would use JUnit Description of method or class name without regex. Cucumber is different runner from most of the other.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user kgyrtkirk commented on the pull request:

          https://github.com/apache/maven-surefire/pull/113#issuecomment-221739676

          I've used `Pattern.compile` to keep the regexp's problem description; i thinked it makes it more clear how it will be used eventually.

          I picked `TestListResolver` as it's place; because this is the first point where the deconstruction logic is used.
          plus: I wanted to keep all the generic exceptions of Pattern prior to these construction exceptions - without reassembling the original regex - but maybe this dosen't matter.

          I might be able to move it to `ResolvedTest`...tomorrow i will look at it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user kgyrtkirk commented on the pull request: https://github.com/apache/maven-surefire/pull/113#issuecomment-221739676 I've used `Pattern.compile` to keep the regexp's problem description; i thinked it makes it more clear how it will be used eventually. I picked `TestListResolver` as it's place; because this is the first point where the deconstruction logic is used. plus: I wanted to keep all the generic exceptions of Pattern prior to these construction exceptions - without reassembling the original regex - but maybe this dosen't matter. I might be able to move it to `ResolvedTest`...tomorrow i will look at it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user kgyrtkirk commented on the pull request:

          https://github.com/apache/maven-surefire/pull/113#issuecomment-222059866

          i've moved the check into `ResolvedTest`.
          I now use the `MatchedPattern`...to check for exceptions.

          I've squashed my changes into one commit; because of the sidetrip to `TestListResolver`

          Show
          githubbot ASF GitHub Bot added a comment - Github user kgyrtkirk commented on the pull request: https://github.com/apache/maven-surefire/pull/113#issuecomment-222059866 i've moved the check into `ResolvedTest`. I now use the `MatchedPattern`...to check for exceptions. I've squashed my changes into one commit; because of the sidetrip to `TestListResolver`
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Tibor17 commented on the issue:

          https://github.com/apache/maven-surefire/pull/113

          @kgyrtkirk Please close the PR. The code was merged with master. Thx for contributing!

          Show
          githubbot ASF GitHub Bot added a comment - Github user Tibor17 commented on the issue: https://github.com/apache/maven-surefire/pull/113 @kgyrtkirk Please close the PR. The code was merged with master. Thx for contributing!
          Hide
          tibor17 Tibor Digana added a comment -

          commit 52681e56db036fa7f648ac43c084ac9f0747213e

          Show
          tibor17 Tibor Digana added a comment - commit 52681e56db036fa7f648ac43c084ac9f0747213e
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build maven-surefire #1600 (See https://builds.apache.org/job/maven-surefire/1600/)
          SUREFIRE-1250 Regex testcase filtering: exception when hashmark is (tibor17: rev 52681e56db036fa7f648ac43c084ac9f0747213e)

          • (edit) surefire-api/src/test/java/org/apache/maven/surefire/testset/TestListResolverTest.java
          • (edit) surefire-api/src/main/java/org/apache/maven/surefire/testset/ResolvedTest.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build maven-surefire #1600 (See https://builds.apache.org/job/maven-surefire/1600/ ) SUREFIRE-1250 Regex testcase filtering: exception when hashmark is (tibor17: rev 52681e56db036fa7f648ac43c084ac9f0747213e) (edit) surefire-api/src/test/java/org/apache/maven/surefire/testset/TestListResolverTest.java (edit) surefire-api/src/main/java/org/apache/maven/surefire/testset/ResolvedTest.java
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user kgyrtkirk closed the pull request at:

          https://github.com/apache/maven-surefire/pull/113

          Show
          githubbot ASF GitHub Bot added a comment - Github user kgyrtkirk closed the pull request at: https://github.com/apache/maven-surefire/pull/113

            People

            • Assignee:
              tibor17 Tibor Digana
              Reporter:
              kgyrtkirk Zoltan Haindrich
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development