Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small patch, adding CharSequence::isEmpty #10097

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

vjay82
Copy link

@vjay82 vjay82 commented Feb 19, 2025

Fixes #10091

@niloc132
Copy link
Member

Thanks for the contribution - would you add a "quick" test for this? The test method itself should be quick (empty string, non-empty string, maybe mutate a StringBuilder to be empty, then non-empty, then empty again), but setting up the test is going to be a little irritating to add since the test won't compile with Java 11, it will need to have super-source.

  • Start with a class like com.google.gwt.emultest.java15.lang.StringTest (can copy from the java11.lang.StringTest for copyright header, superclass), but it will need a special runTest method like Java17Test has to only run with a high enough source level.
  • The test method itself needs to be a stub with no implementation, since it wouldn't compile otherwise.
  • Then, make a copy of the class, with no runTest, and with the stub populated, in somewhere like user/test-super/com/google/gwt/dev/jjs/super/com/google/gwt/emultest/java15/lang/StringTest.java. This will be supersourced, so your IDE may not see it properly, and will correctly not be compiled in Java 11.

@vjay82
Copy link
Author

vjay82 commented Feb 20, 2025

Ok, will have a look when some free time is at hand,

@vjay82
Copy link
Author

vjay82 commented Mar 2, 2025

Hi,

I implement the tests as instructed.

Then I tried to let them run according to the instructions in the README.md.

Using Java 21:

set TZ=America/Los_Angeles
set ANT_OPTS=-Dfile.encoding=UTF-8
ant clean
ant user -Dtarget=test
...
[junit] Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0,721 sec
[junit] Test com.google.gwt.dev.javac.PersistentUnitCacheTest FAILED
...
[junit] Tests run: 24, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 19,753 sec
[junit] Test com.google.gwt.dev.jjs.impl.DeadCodeEliminationTest FAILED
...
BUILD FAILED

My new com.google.gwt.emultest.java15.lang.StringTest was never called. (Wouldn't CharSequenceTest be a better name?)

I checked where com.google.gwt.emultest.java11.lang.StringTest might be called/linked and found com.google.gwt.emultest.EmulJava11Suite.

I created a corresponding EmulJava15Suite but still no change.

I also checked if EmulJava11Suite is refered to anywhere, which is not the case.

The java11 StringTest seems also not to be listed (probably not called?) in the output = ?

I set my test up to fail:

com.google.gwt.emultest.java15.lang.StringTest extends EmulTestBase {
	
  public void testIsEmpty() {
    // empty stub
    assertFalse(true);
  }

}

(Also broke gwt\user\test-super\com\google\gwt\dev\jjs\super\com\google\gwt\dev\jjs\super\com\google\gwt\emultest\java15\lang\StringTest.java)

The output of the ant-call was the same.

I then tried to set up ...java11.lang.StringTest to fail.
Same output again.

Ich checked if DeadCodeEliminationTest (which is statet in the output) is referred to anywhere. It is not...

Finally, as I might have broken something, I reverted my changes back and ran the tests on the original repo:

set TZ=America/Los_Angeles
set ANT_OPTS=-Dfile.encoding=UTF-8
ant clean
ant user -Dtarget=test
...
[junit] Tests run: 5, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0,68 sec
[junit] Test com.google.gwt.dev.javac.PersistentUnitCacheTest FAILED
...
[junit] Tests run: 24, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 18,227 sec
[junit] Test com.google.gwt.dev.jjs.impl.DeadCodeEliminationTest FAILED
...
BUILD FAILED

Exactly the same thing.

I tried to run "all" tests without "user" prefix - to make sure every single one should be run.
The command also fails, see all tests.txt

Any ideas?

@niloc132
Copy link
Member

niloc132 commented Mar 2, 2025

Would you push the commit with the draft test so I can take a look there, give it a try?

The way I run tests is usually based on the table, cd'ing into the directory and running a task or two, like this:

cd user/
ant test.web.htmlunit '-Dgwt.junit.testcase.web.includes=**/CharSequenceTest.class'

that will run every test with that name in user/ with htmlunit, with a production build (rather than draft, or disabling class metadata) - for a simple emulation test like this, that should be sufficient.

With your own github fork, you can also push a branch name with a -test suffix, and enable github actions for your fork - the entire test suite will run. Name your branch something like charsequence-isempty-test and push that to your fork.


I'm very far from an ant expert, and as far as I can tell that ant dev -Dtarget=test is nonsense - if you specify another task in dev but not other subprojects (like buildtools), it will fail right away when buildtools can't use it. It seems to be running the target task everywhere...rather than just in the specified project.

As to why those tests are failing for you locally, I'm not sure, do you want to open a separate issue for that? I would guess there is a windows specific issue with how the tests are defined.

@jnehlmeier
Copy link
Member

DeadCodeEliminationTest is likely failing because of the Java default locale used on the system. There is a test that assumes US/en locale and it seems @vjay82 is located in Germany, like me. See: #10067

You have to run the tests using -Duser.country=US -Duser.language=en

@jnehlmeier
Copy link
Member

Also if you are using macOS some tests will simply fail, because Java on macOS has a bad implementation of WatchService. See: #10069

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CharSequence isEmpty method is missing
3 participants