TestUnicodeInvariants: add tests for TestCodeInvariants #247

srl295 · 2022-05-11T18:07:58Z

adds tests for these classes

TestUnicodeInvariants
TestCodeInvariants

srl295 · 2022-05-11T18:29:34Z

fix path issue

java.io.FileNotFoundException: /home/runner/work/unicodetools/unicodetools/unicodetools/nullorg/unicode/text/UCD/UnicodeInvariantTest.txt (No such file or directory)
[2784](https://github.com/unicode-org/unicodetools/runs/6393672630?check_suite_focus=true#step:8:2784)

also @markusicu in the best case there are still 3 failures. Make it a warning for now?

markusicu · 2022-05-11T18:48:06Z

fix path issue

The file is there, and the test runs fine in my workspace. Is the test run missing a variable definition for the UNICODETOOLS_REPO_DIR ?

https://github.com/unicode-org/unicodetools/blob/main/unicodetools/src/main/resources/org/unicode/text/UCD/UnicodeInvariantTest.txt

also @markusicu in the best case there are still 3 failures. Make it a warning for now?

That's what we get for not running it every time...

I just ran it locally and got one test failure. I will send an email.

\p{lb=EB} = \p{Emoji_Modifier_Base}
**** START Test Failure 1 ****
## Expected empty, got: 2	[\U0001FAF7\U0001FAF8]
## In	\p{Emoji_Modifier_Base}
## But Not In	\p{lb=EB} 
1FAF7..1FAF8   #  [2] (�..�)  LEFTWARDS PUSHING HAND..RIGHTWARDS PUSHING HAND
**** END Test Failure 1 ****

markusicu · 2022-05-11T22:18:43Z

also @markusicu in the best case there are still 3 failures. Make it a warning for now?

That's what we get for not running it every time...

I just ran it locally and got one test failure. I will send an email.

TestUnicodeInvariants runs cleanly after PR #249. Please rebase.

srl295 · 2022-05-11T22:45:28Z

fix path issue

The file is there, and the test runs fine in my workspace.

mine also. will investigate

…deInvariants - did not move code into the test subdir..yet

- add a test for SRC_UCD_DIR not containing nulls - fix a misnamed "outputFile" that was really an input file #186

- read data from classpath on null. #186

srl295 · 2022-05-11T23:29:44Z

@markusicu couldn't sort it out. Changed to use classpath loader.

markusicu · 2022-05-12T03:13:20Z

unicodetools/src/main/java/org/unicode/text/UCD/TestUnicodeInvariants.java

-    public static void testInvariants(String outputFile, boolean doRange) throws IOException {
+    /**
+     * Fetch a reader for our input data.
+     * @param inputFile if null, read DEFAULT_FILE from classpath


Why would it be null? Line 80 calls testInvariants() with DEFAULT_FILE, or with a String from a command-line argument, if there is one.

I pass null from TestTestUnicodeInvariants to mean default

How about passing TestUnicodeInvariants.DEFAULT_FILE from the TestTest... code?

markusicu · 2022-05-12T03:20:23Z

unicodetools/src/test/java/org/unicode/text/UCD/TestTestCodeInvariants.java

+
+import org.junit.jupiter.api.Test;
+
+public class TestTestCodeInvariants {


These two TestTest... classes seem awkward.
I see in the PR description "Does not refactor code into the test area, yet" -- is that the next step for this PR? Moving the regular files into the test path and changing them directly, to JUnit tests, replacing these files?

@markusicu it's because there are other functions in the original class. Is it called from elsewhere, is it run directly with a parameter? If I remove the original class from src/main/java then it can't be called as easily.

If this looks right I can merge this and then reduce the test classes in a future PR.

srl295 · 2022-05-12T16:50:01Z

@markusicu I've removed TestTestCodeInvariants

But, TestUnicodeInvariants can be called with options to give it a different input file, to output HTML, to skip ranges, etc. So I will leave that in the main/java subtree unless i hear otherwise.

- move TestCodeInvariants into the test/ subtree

markusicu

But, TestUnicodeInvariants can be called with options to give it a different input file, to output HTML, to skip ranges, etc. So I will leave that in the main/java subtree unless i hear otherwise.

Hm. I don't like the TestTest... name, but I see your point. The alternative could be to strip the one Test from the old class where it is and use the old name on the new test-package class. But that's less friendly to file history forensics, and the class-with-main() is still kind of a test. So... ok.

But I have some other comments.

unicodetools/src/test/java/org/unicode/text/UCD/TestCodeInvariants.java

markusicu · 2022-05-12T18:30:30Z

unicodetools/src/test/java/org/unicode/text/UCD/TestCodeInvariants.java

@@ -41,12 +44,24 @@ public static void main(String[] args) {
        testGcbInDecompositions(true);


I would remove the showAllNfds parameter from this function and change its code as if it had been false (or replace showAllNfds with VERBOSE). It seems unnecessary to do verbose printing for thousands of decompositions that pass the test.

markusicu · 2022-05-12T18:33:07Z

unicodetools/src/main/java/org/unicode/text/UCD/TestUnicodeInvariants.java

-    public static void testInvariants(String outputFile, boolean doRange) throws IOException {
+    /**
+     * Fetch a reader for our input data.
+     * @param inputFile if null, read DEFAULT_FILE from classpath


How about passing TestUnicodeInvariants.DEFAULT_FILE from the TestTest... code?

unicodetools/src/main/java/org/unicode/text/UCD/TestUnicodeInvariants.java

markusicu · 2022-05-12T18:43:58Z

unicodetools/src/main/java/org/unicode/text/UCD/TestUnicodeInvariants.java

+     */
+    private static BufferedReader getInputReader(String inputFile) throws IOException {
+        if (inputFile != null) {
+            return FileUtilities.openUTF8Reader(Settings.SRC_UCD_DIR, inputFile);


FYI This should work in the build bots. For example, https://github.com/unicode-org/unicodetools/blob/main/.github/workflows/build-jsp.yml#L52 does set -DUNICODETOOLS_REPO_DIR=$(pwd), and Settings.SRC_UCD_DIR is relative to that. We read all source data files relative to that path.

should but does not

- remove dead code and functions such as main() - an earlier iteration did not call the GCB tests properly from JUnit #186

markusicu

Ok except for one more thing...

unicodetools/src/test/java/org/unicode/text/UCD/TestCodeInvariants.java

- an earlier iteration still did not call the GCB tests properly from JUnit #186

markusicu

lgtm tnx -- please remember to squash-and-merge as usual

srl295 requested a review from markusicu May 11, 2022 18:07

srl295 self-assigned this May 11, 2022

srl295 force-pushed the srl295/issue186c branch from 7ec984b to 0774db9 Compare May 11, 2022 18:19

srl295 marked this pull request as ready for review May 11, 2022 18:20

srl295 added 2 commits May 11, 2022 17:46

TestUnicodeInvariants: add tests for TestCodeInvariants and TestUnico…

2bd8c54

…deInvariants - did not move code into the test subdir..yet

TestUnicodeInvariants: test improvements

a0d2657

- add a test for SRC_UCD_DIR not containing nulls - fix a misnamed "outputFile" that was really an input file #186

srl295 force-pushed the srl295/issue186c branch from 0774db9 to a0d2657 Compare May 11, 2022 22:58

TestUnicodeInvariants: test improvements

56627a6

- read data from classpath on null. #186

markusicu reviewed May 12, 2022

View reviewed changes

srl295 requested a review from markusicu May 12, 2022 16:50

TestUnicodeInvariants: test improvements

32199e9

- move TestCodeInvariants into the test/ subtree

srl295 force-pushed the srl295/issue186c branch from 87f2eda to 32199e9 Compare May 12, 2022 16:58

markusicu reviewed May 12, 2022

View reviewed changes

TestCodeInvariants: test improvements

dc7466f

- remove dead code and functions such as main() - an earlier iteration did not call the GCB tests properly from JUnit #186

srl295 requested a review from markusicu May 12, 2022 19:26

markusicu reviewed May 12, 2022

View reviewed changes

unicodetools/src/test/java/org/unicode/text/UCD/TestCodeInvariants.java Show resolved Hide resolved

TestCodeInvariants: add a @test annotation

beed3cc

- an earlier iteration still did not call the GCB tests properly from JUnit #186

markusicu approved these changes May 12, 2022

View reviewed changes

srl295 merged commit a4f7ab1 into main May 12, 2022

srl295 deleted the srl295/issue186c branch May 12, 2022 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TestUnicodeInvariants: add tests for TestCodeInvariants #247

TestUnicodeInvariants: add tests for TestCodeInvariants #247

srl295 commented May 11, 2022 •

edited by markusicu

Loading

srl295 commented May 11, 2022 •

edited

Loading

markusicu commented May 11, 2022 •

edited by srl295

Loading

markusicu commented May 11, 2022

srl295 commented May 11, 2022

srl295 commented May 11, 2022

markusicu May 12, 2022

srl295 May 12, 2022

markusicu May 12, 2022

markusicu May 12, 2022

srl295 May 12, 2022

srl295 commented May 12, 2022 •

edited

Loading

markusicu left a comment

markusicu May 12, 2022

srl295 May 12, 2022

markusicu May 12, 2022

markusicu May 12, 2022

srl295 May 12, 2022

markusicu left a comment

markusicu left a comment


		import org.junit.jupiter.api.Test;

		public class TestTestCodeInvariants {

		@@ -41,12 +44,24 @@ public static void main(String[] args) {
		testGcbInDecompositions(true);

TestUnicodeInvariants: add tests for TestCodeInvariants #247

TestUnicodeInvariants: add tests for TestCodeInvariants #247

Conversation

srl295 commented May 11, 2022 • edited by markusicu Loading

srl295 commented May 11, 2022 • edited Loading

markusicu commented May 11, 2022 • edited by srl295 Loading

markusicu commented May 11, 2022

srl295 commented May 11, 2022

srl295 commented May 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 commented May 12, 2022 • edited Loading

markusicu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusicu left a comment

Choose a reason for hiding this comment

markusicu left a comment

Choose a reason for hiding this comment

srl295 commented May 11, 2022 •

edited by markusicu

Loading

srl295 commented May 11, 2022 •

edited

Loading

markusicu commented May 11, 2022 •

edited by srl295

Loading

srl295 commented May 12, 2022 •

edited

Loading