In general, not too bad. However: 1) the comment above is a rationale, but not a design doc. Please write a design doc that explains how this solves the problem not just why it is needed. Not being terribly familar with the JUnit infrastructure, it took me a while to mostly figure out how it works.
2) I think this is enough new functionality we need to get team agreement on what is being added. Please create an RFC wave to do so. http://gwt-code-reviews.appspot.com/1043801/diff/1/4 File dev/core/src/com/google/gwt/dev/cfg/SyntheticModuleDef.java (right): http://gwt-code-reviews.appspot.com/1043801/diff/1/4#newcode50 dev/core/src/com/google/gwt/dev/cfg/SyntheticModuleDef.java:50: public synchronized String getPrimaryModuleName() { I think rather than changing all the references of ModuleDef to SyntheticModuleDef, you should just add this functionality into ModuleDef. http://gwt-code-reviews.appspot.com/1043801/diff/1/5 File tools/api-checker/config/gwt20_21userApi.conf (right): http://gwt-code-reviews.appspot.com/1043801/diff/1/5#newcode73 tools/api-checker/config/gwt20_21userApi.conf:73: :user/src/com/google/gwt/junit/client/TestServiceFactory.java\ Why are these excluded? If they aren't in impl packages, we are saying we will support the API. http://gwt-code-reviews.appspot.com/1043801/diff/1/5#newcode148 tools/api-checker/config/gwt20_21userApi.conf:148: com.google.gwt.junit.client.impl.GWTRunner MISSING Maybe all classes in impl packages should be excluded (around line 87) instead. http://gwt-code-reviews.appspot.com/1043801/diff/1/7 File user/src/com/google/gwt/junit/E2eJUnitShell.java (right): http://gwt-code-reviews.appspot.com/1043801/diff/1/7#newcode80 user/src/com/google/gwt/junit/E2eJUnitShell.java:80: options.setProxyToHost(getHost()); Why do we need this? http://gwt-code-reviews.appspot.com/1043801/diff/1/8 File user/src/com/google/gwt/junit/JUnitShell.java (right): http://gwt-code-reviews.appspot.com/1043801/diff/1/8#newcode151 user/src/com/google/gwt/junit/JUnitShell.java:151: OptionProxyTo { Formatting http://gwt-code-reviews.appspot.com/1043801/diff/1/8#newcode1140 user/src/com/google/gwt/junit/JUnitShell.java:1140: mapping.setServletName(defaultServlet.getName()); Where do you override the existing default servlet? http://gwt-code-reviews.appspot.com/1043801/diff/1/8#newcode1536 user/src/com/google/gwt/junit/JUnitShell.java:1536: // TODO: figure out if it is possible to allow multiple modules. I think this needs to support multiple modules. http://gwt-code-reviews.appspot.com/1043801/diff/1/13 File user/src/com/google/gwt/junit/client/GWTTestCase.java (right): http://gwt-code-reviews.appspot.com/1043801/diff/1/13#newcode199 user/src/com/google/gwt/junit/client/GWTTestCase.java:199: // implemented in the translatable version of this class Where is the translatable version? http://gwt-code-reviews.appspot.com/1043801/diff/1/14 File user/src/com/google/gwt/junit/client/TestService.java (right): http://gwt-code-reviews.appspot.com/1043801/diff/1/14#newcode25 user/src/com/google/gwt/junit/client/TestService.java:25: public interface TestService { The name could probably be improved. http://gwt-code-reviews.appspot.com/1043801/diff/1/14#newcode29 user/src/com/google/gwt/junit/client/TestService.java:29: void click(HasClickHandlers hasClickHandlers); It seems odd that all we have here is click. http://gwt-code-reviews.appspot.com/1043801/diff/1/16 File user/src/com/google/gwt/junit/client/impl/GWTRunner.java (right): http://gwt-code-reviews.appspot.com/1043801/diff/1/16#newcode45 user/src/com/google/gwt/junit/client/impl/GWTRunner.java:45: AsyncCallback<InitialResponse> { Is this mostly the same as the contents of the translatable version that was deleted? Why do we no longer need a separate translatable version? http://gwt-code-reviews.appspot.com/1043801/diff/1/18 File user/src/com/google/gwt/junit/server/ProxyFilter.java (right): http://gwt-code-reviews.appspot.com/1043801/diff/1/18#newcode37 user/src/com/google/gwt/junit/server/ProxyFilter.java:37: * EXPERIMENTAL and subject to change. What is experimental about it? Since external users have been requesting the ability to proxy to an external server, is there any reason we would encourage them to use this? http://gwt-code-reviews.appspot.com/1043801/diff/1/18#newcode127 user/src/com/google/gwt/junit/server/ProxyFilter.java:127: return replaceModuleName ? uri.replaceFirst(syntheticModuleName, replaceFirst takes regexes, but you are using literals. I think this would be better implemented like: String uri = super.getRequestURI(); if (uri.startsWith("/") && syntheticModuleName != null) { int n = syntheticModuleName.length(); if (syntheticModuleName.equals(uri.substring(1, n + 1))) { uri = "/" + moduleName + uri.substring(n + 1); } } return uri; http://gwt-code-reviews.appspot.com/1043801/diff/1/18#newcode158 user/src/com/google/gwt/junit/server/ProxyFilter.java:158: context.log("ProxyFilter.doFilter on proxied URL"); Better log message. http://gwt-code-reviews.appspot.com/1043801/diff/1/18#newcode168 user/src/com/google/gwt/junit/server/ProxyFilter.java:168: public void setModuleName(String moduleName, String syntheticModuleName) { Javadoc http://gwt-code-reviews.appspot.com/1043801/diff/1/18#newcode171 user/src/com/google/gwt/junit/server/ProxyFilter.java:171: context.log("ProxyFilter.setModuleName(" + moduleName + ", " Does this need to be logged? I am not sure what level this gets logged at, but if it is INFO then I don't think we want to spam the log with messages like this. If so, it needs a better message. http://gwt-code-reviews.appspot.com/1043801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
