fix: improves method name handling executors

[erdi]

Short description of what this resolves:

This resolves #214 by adding support for JUnit compatible testing frameworks for which FrameworkMethod.getName() != FrameworkMethod.getMethod().getName() and which allow test method names with reserved url characters.

Changes proposed in this pull request:

• Use the logical and not the actual test method name in ServletMethodExecutor
• Escape reserved characters in test method names when composing urls in ServletMethodExecutor

Additional notes

I understand that this fixes two separate issues but I decided to submit fixes for them as a single PR because I believe that are interrelated. Please let me know if you'd like to have this split into two commits and two PRs.

I believe that changes made are backwards compatible with the only problematic one in my opinion being introduction of TestMethodExecutor.getMethodName(). Ideally it would be added as a default method but Arquillian is still Java 6 compatible so it's not possible. I'm not sure if users are expected to have custom TestMethodExecutor implementation but if they are then I'm looking forward on suggestions on how this could be implemented without the potentially breaking change.

I have added a test for my changes, tried to keep the changes to minimum and follow the conventions set out in the project. Please do let me know if I missed something and I will gladly apply any necessary changes to this PR.

[erdi]

CI failure seems to be unrelated to my changes. Locally I have successfully run the following:

./mvnw install -q -U -DskipTests=true
./mvnw clean install
...
[INFO] Reactor Summary for Arquillian Aggregator 1.4.2.Final-SNAPSHOT:
[INFO] 
[INFO] Arquillian Aggregator .............................. SUCCESS [  1.201 s]
[INFO] Arquillian Build ................................... SUCCESS [  0.552 s]
[INFO] Arquillian Core API ................................ SUCCESS [  1.194 s]
[INFO] Arquillian Core SPI ................................ SUCCESS [  0.611 s]
[INFO] Arquillian Core Implementation Base ................ SUCCESS [  1.976 s]
[INFO] Arquillian Core Aggregator ......................... SUCCESS [  0.124 s]
[INFO] Arquillian Config API .............................. SUCCESS [  1.173 s]
[INFO] Arquillian Config SPI .............................. SUCCESS [  0.447 s]
[INFO] Arquillian Test API ................................ SUCCESS [  0.431 s]
[INFO] Arquillian Test SPI ................................ SUCCESS [  1.657 s]
[INFO] Arquillian Test Implementation Base ................ SUCCESS [  1.519 s]
[INFO] Arquillian Config Implementation Base .............. SUCCESS [  3.366 s]
[INFO] Arquillian Config Aggregator ....................... SUCCESS [  0.249 s]
[INFO] Arquillian Test Aggregator ......................... SUCCESS [  0.100 s]
[INFO] Arquillian Container SPI ........................... SUCCESS [  1.497 s]
[INFO] Arquillian Container Implementation Base ........... SUCCESS [  3.230 s]
[INFO] Arquillian Container Test API ...................... SUCCESS [  1.194 s]
[INFO] Arquillian Container Test SPI ...................... SUCCESS [  0.635 s]
[INFO] Arquillian Container Test Implementation Base ...... SUCCESS [  2.406 s]
[INFO] Arquillian Container Aggregator .................... SUCCESS [  0.248 s]
[INFO] Arquillian TestRunner JUnit Core ................... SUCCESS [  3.553 s]
[INFO] Arquillian TestRunner JUnit Standalone ............. SUCCESS [  1.871 s]
[INFO] Arquillian TestRunner JUnit Container .............. SUCCESS [  2.063 s]
[INFO] Arquillian TestRunner JUnit Aggregator ............. SUCCESS [  0.109 s]
[INFO] Arquillian TestRunner TestNG Core .................. SUCCESS [  2.530 s]
[INFO] Arquillian TestRunner TestNG Standalone ............ SUCCESS [  0.453 s]
[INFO] Arquillian TestRunner TestNG Container ............. SUCCESS [  2.312 s]
[INFO] Arquillian TestRunner TestNG Aggregator ............ SUCCESS [  0.118 s]
[INFO] Arquillian TestEnricher CDI ........................ SUCCESS [  3.148 s]
[INFO] Arquillian TestEnricher EJB ........................ SUCCESS [  1.897 s]
[INFO] Arquillian TestEnricher Resource ................... SUCCESS [  1.869 s]
[INFO] Arquillian TestEnricher InitialContext ............. SUCCESS [  0.741 s]
[INFO] Arquillian TestEnricher Aggregator ................. SUCCESS [  0.111 s]
[INFO] Arquillian Protocol Servlet 2.5/3.x ................ SUCCESS [  4.195 s]
[INFO] Arquillian Protocol JMX ............................ SUCCESS [  1.401 s]
[INFO] Arquillian Protocol Aggregator ..................... SUCCESS [  0.114 s]
[INFO] Arquillian BOM ..................................... SUCCESS [  0.237 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  22.297 s (Wall Clock)
[INFO] Finished at: 2019-09-20T13:30:49+01:00
[INFO] ------------------------------------------------------------------------

[bartoszmajsak]

Thank you very much for this awesome contribution. I really do appreciate an effort not only in going through the code base and fixing it but also all the comments and thoughts you shared. 🥇

As for the new method and breaking backward compatibility - most of the test runners are under this org umbrella so I can take care of it. For the others (and there are not that many I think) - how about we mark the old getMethod @Depracated pointing to why the new one should be used in Javadoc?

[bartoszmajsak]

CI failure seems to be unrelated to my changes. Locally I have successfully run the following:

Yeah, Travis is getting unreliable lately. Don't worry, I will have a look now.

In parallel, I'm thinking about moving to CircleCI or GH Actions.

[erdi]

Thanks for noticing my efforts and finding my contribution valuable. As an OSS project maintainer myself I always try my best on PRs to other projects so that it's as easy as possible for the maintainers to review. Putting in the effort also increases the chances of the contribution being accepted and in the end the main driver here is to get the issue I'm affected by fixed. 😃

[erdi]

For the others (and there are not that many I think) - how about we mark the old getMethod @Depracated pointing to why the new one should be used in Javadoc?

Hmmm... I don't quite follow. Why would we want to deprecate TestMethodExecutor.getMethod()? I believe it's still needed and rightly so. It still has its use, in org.jboss.arquillian.container.test.impl.execution.LocalTestExecuter#execute for example as well as in org.jboss.arquillian.protocol.servlet.ServletMethodExecutor#invoke which passes the result to org.jboss.arquillian.protocol.servlet.ServletURIHandler#locateTestServlet(). Would you disagree?

Btw, I've noticed a couple of issues in this PR (two implementations of TestMethodExecutor.getMethodName() are incorrect and in some places TestMethodExecutor.getMethod().getName() is still used instead of TestMethodExecutor.getMethodName()). I'd therefore like to push an additional commit to this branch but you've already merged master into it. Would you be ok with me adding a commit, dropping your merge commits and rebasing my branch onto the top of current master?

[bartoszmajsak]

Hmmm... I don't quite follow. Why would we want to deprecate TestMethodExecutor.getMethod()? I believe it's still needed and rightly so

Correct, I think I was a bit too fast.

I'd therefore like to push an additional commit to this branch but you've already merged master into it. Would you be ok with me adding a commit, dropping your merge commits and rebasing my branch onto the top of current master?

I think you can simply add another commit and I will take care of bringing those changes back to master, don't worry about that.

[erdi]

Ok, I believe I have addressed all of your comments, over to you, @bartoszmajsak.

[bartoszmajsak]

Thanks @erdi, I will run a few tests in combination with another project and cut the release today. Many thanks for your contribution.