CLDR-16393 assume cldr-archive is present for tests #3215

srl295 · 2023-08-23T18:14:26Z

UNLESS -DNO_CLDR_ARCHIVE=true is set

This PR completes the ticket.

This will cause tests to FAIL unless:

cldr-archive is setup OR
-DNO_CLDR_ARCHIVE=true is set.

ALLOW_MANY_COMMITS=true

⚠️ if we merge this, users will start getting failures unless they have an up-to-date cldr-archive OR disable the check as described here.

jira-pull-request-webhook · 2023-08-23T23:43:37Z

Notice: the branch changed across the force-push!

tools/cldr-code/src/test/java/org/unicode/cldr/util/TestCLDRPaths.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

srl295 · 2023-08-24T00:15:11Z

old tests fail though,

Error:  (TestBasic.java:1454)  Error: : ldml DTD - Trunk children of «zone» must be superset of v1.5.1, and must contain «usesMetazone»: expected != null

Error:  (TestBasic.java:1454)  Error: : ldml DTD - Trunk children of «zone» must be superset of v1.5.0.1, and must contain «usesMetazone»: expected != null

Error:  (TestBasic.java:1454)  Error: : ldmlICU DTD - Trunk children of «zone» must be superset of v1.5.1, and must contain «usesMetazone»: expected != null

Error:  (TestBasic.java:1454)  Error: : ldmlICU DTD - Trunk children of «zone» must be superset of v1.5.0.1, and must contain «usesMetazone»: expected != null

maybe a new logknown issue to skip the failing ones?

srl295 · 2023-08-26T13:49:41Z

old tests fail though,

Error:  (TestBasic.java:1454)  Error: : ldml DTD - Trunk children of «zone» must be superset of v1.5.1, and must contain «usesMetazone»: expected != null

Error:  (TestBasic.java:1454)  Error: : ldml DTD - Trunk children of «zone» must be superset of v1.5.0.1, and must contain «usesMetazone»: expected != null

Error:  (TestBasic.java:1454)  Error: : ldmlICU DTD - Trunk children of «zone» must be superset of v1.5.1, and must contain «usesMetazone»: expected != null

Error:  (TestBasic.java:1454)  Error: : ldmlICU DTD - Trunk children of «zone» must be superset of v1.5.0.1, and must contain «usesMetazone»: expected != null

maybe a new logknown issue to skip the failing ones?

I'll probably do this, but later

jira-pull-request-webhook · 2023-08-30T22:52:40Z

Notice: the branch changed across the force-push!

tools/cldr-code/src/main/java/org/unicode/cldr/tool/CheckHtmlFiles.java is now changed in the branch
tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateDtd.java is now changed in the branch
tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateItemCounts.java is now changed in the branch
tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateReformattedXml.java is now changed in the branch
tools/cldr-code/src/main/java/org/unicode/cldr/tool/ShowStarredCoverage.java is now changed in the branch
tools/cldr-code/src/main/java/org/unicode/cldr/util/DtdType.java is different
tools/cldr-code/src/main/java/org/unicode/cldr/util/SupplementalDataInfo.java is different
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestBasic.java is different
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestComparisonBuilder.java is now changed in the branch
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestDTDAttributes.java is now changed in the branch
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestDtdData.java is now changed in the branch
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestPathHeader.java is now changed in the branch
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestPaths.java is now changed in the branch
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestValidity.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

macchiati · 2023-08-30T23:04:12Z

Looks like roughly the right direction, but we are running short of time; I will check in later quickly, but basically I'm done through the 6th and you through the 15th.

We will need to regenerate charts and do other things that depend on platforms (which are far more timely than Subdivisiions) so I suggest again that you restore platforms with a dummy file so that we can proceed.

srl295 · 2023-08-30T23:12:44Z

Looks like roughly the right direction, but we are running short of time; I will check in later quickly, but basically I'm done through the 6th and you through the 15th.

We will need to regenerate charts and do other things that depend on platforms (which are far more timely than Subdivisiions) so I suggest again that you restore platforms with a dummy file so that we can proceed.

There are other bugs in the unit tests besides platforms. It's not respecting the firstVersion parameter.

This PR currently knows about but skips DtdType.platform in all loops, is that sufficient ?what process "needs" platforms?

macchiati · 2023-08-30T23:54:23Z

No process uses it; most of the attributeValidity is no longer used.
I'm just leery of such a big change at a bad time.

srl295 · 2023-08-31T00:04:24Z

No process uses it; most of the attributeValidity is no longer used.

I'm just leery of such a big change at a bad time.

Mark, I hear you; but there are other major breakages in the tests. FirstVersion is ignored in many cases. ElementInfo also ignores firstVersion.

Can you comment on the unit comparator "jp-bu" I think it was ?

macchiati · 2023-08-31T00:41:45Z

Enabling the tests to work across all versions is a worthy goal. But it is *not* a show stopper for this release. You have very little time until you are gone, so we want to make the most of it. The separate, immediate goal is to get GenerateSubdivisions to work. For that, I think the only thing I think we need is to restore the platform enum value and dtd file.

…

On Wed, Aug 30, 2023 at 5:04 PM Steven R. Loomis ***@***.***> wrote: No process uses it; most of the attributeValidity is no longer used. I'm just leery of such a big change at a bad time. Mark, I hear you; but there are other major breakages in the tests. FirstVersion is ignored in many cases. ElementInfo also ignores firstVersion. Can you comment on the unit comparator "jp-bu" I think it was ? — Reply to this email directly, view it on GitHub <#3215 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMD63Z5HN4KPQFWS5JTXX7IJHANCNFSM6AAAAAA333SHUY> . You are receiving this because your review was requested.Message ID: ***@***.***>

macchiati · 2023-08-31T00:44:46Z

Can you comment on the unit comparator "jp-bu" I think it was ?

I don't have enough context to know what you mean by this. There is a unit comparator, and there is a bu-jp unit that it will compare (below), but I don't know what the problem is. <convertUnit source='bu-jp' baseUnit='square-meter' factor='tsubo_to_m2*1' systems="jpsystem"/>

…

On Wed, Aug 30, 2023 at 5:41 PM Mark Davis Ⓤ ***@***.***> wrote: Enabling the tests to work across all versions is a worthy goal. But it is *not* a show stopper for this release. You have very little time until you are gone, so we want to make the most of it. The separate, immediate goal is to get GenerateSubdivisions to work. For that, I think the only thing I think we need is to restore the platform enum value and dtd file. On Wed, Aug 30, 2023 at 5:04 PM Steven R. Loomis ***@***.***> wrote: > No process uses it; most of the attributeValidity is no longer used. > > I'm just leery of such a big change at a bad time. > > Mark, I hear you; but there are other major breakages in the tests. > FirstVersion is ignored in many cases. ElementInfo also ignores > firstVersion. > > Can you comment on the unit comparator "jp-bu" I think it was ? > > — > Reply to this email directly, view it on GitHub > <#3215 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACJLEMD63Z5HN4KPQFWS5JTXX7IJHANCNFSM6AAAAAA333SHUY> > . > You are receiving this because your review was requested.Message ID: > ***@***.***> >

srl295 · 2023-08-31T01:34:24Z

@macchiati this is the next issue hit, in the code that GenerateSubdivisions (and others) will hit, that the bu-jp units aren't present prior to v44.

srl295 · 2023-08-31T01:35:47Z

Can you comment on the unit comparator "jp-bu" I think it was ?
I don't have enough context to know what you mean by this. There is a unit comparator, and there is a bu-jp unit that it will compare (below), but I don't know what the problem is.
…

the bu-jp units aren't present in units.xml prior to v44, that seems to be the source of the failure.

I'm fine to table this for now if it's off the critical path.

macchiati · 2023-08-31T03:29:08Z

Lot's of units come into new versions, so it must be something special about that one.

srl295 · 2023-08-31T03:57:14Z

But do you agree to put this on ice for the moment?

macchiati · 2023-08-31T14:54:16Z

Yes, I agree. Since you are going to be out in car there are problems.

…

On Wed, Aug 30, 2023, 20:57 Steven R. Loomis ***@***.***> wrote: But do you agree to put this on ice for the moment? — Reply to this email directly, view it on GitHub <#3215 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMAPRSXIGLWHXCTRF6TXYADSJANCNFSM6AAAAAA333SHUY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

- UNLESS -DNO_CLDR_ARCHIVE=true is set

- also, reinstate the 'platform' DtdType as 'removed'

- skip ldmlICU versions that are missing - ElementAttributeInfo needs to skip XML files that are missing

jira-pull-request-webhook · 2023-11-29T20:57:17Z

Notice: the branch changed across the force-push!

tools/cldr-code/src/main/java/org/unicode/cldr/tool/CldrVersion.java is different
tools/cldr-code/src/main/java/org/unicode/cldr/util/DtdType.java is different
tools/cldr-code/src/main/java/org/unicode/cldr/util/ElementAttributeInfo.java is different
tools/cldr-code/src/main/java/org/unicode/cldr/util/SupplementalDataInfo.java is different
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestBasic.java is different
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestComparisonBuilder.java is different
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestDtdData.java is different
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestPathHeader.java is different
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestPaths.java is different
tools/cldr-code/src/test/java/org/unicode/cldr/unittest/TestTransforms.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

srl295 · 2023-11-29T20:59:51Z

just rebasing this PR in my queue.

srl295 · 2023-11-29T21:01:12Z

this was affected by CLDR-17078 rename ldmlKeyboard.dtd to ldmlKeyboard3.dtd #3274 because we are retaining the old DTDs. It doesn't attempt to have new code test the old DTDs.

srl295 · 2023-11-29T21:02:59Z

@macchiati when this merges, you will be able to create new tests that use cldr-archive with the following snippet. No need to warn or anything as warning is already handled globally.

        if (!TestCLDRPaths.canUseArchiveDirectory()) {
            return; // skipped
        }

        // ... use cldr-archive ...

JUnit tests can use:

        assumeTrue(TestCLDRPaths.canUseArchiveDirectory());

… which will show the entire test as skipped, providing a better flag to the user.

srl295 · 2023-11-29T21:06:22Z

Can you comment on the unit comparator "jp-bu" I think it was ?
I don't have enough context to know what you mean by this. There is a unit comparator, and there is a bu-jp unit that it will compare (below), but I don't know what the problem is.

this issue didn't surface with current code/data.

macchiati

Nice work. Just a few tweaks needed.

macchiati · 2023-11-29T21:07:56Z

tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateDtd.java

@@ -21,6 +21,7 @@ public static void main(String[] args) throws IOException {
        final DTD2Markdown dtd2md = new DTD2Markdown();
        // System.setProperty("show_all", "true");
        for (DtdType type : DtdType.values()) {
+            if (type.getStatus() != DtdType.DtdStatus.active) continue;


Missing {...}

This happens multiple times below, so check for those also.

tools/cldr-code/src/test/java/org/unicode/cldr/util/TestCLDRPaths.java

macchiati · 2023-11-29T21:20:12Z

tools/cldr-code/src/test/java/org/unicode/cldr/util/TestCLDRPaths.java

+                    String.format(
+                            "Could not read archive directory %s. "
+                                    + "Please: "
+                                    + "1) setup the archive, <https://cldr.unicode.org/development/creating-the-archive>, "


Ambiguous: does this mean #1 and (#2 or #3), or (#1 and #2) or #3?

i've changed this (see example run below).

I took out the option of changing -DARCHIVE by instead folding that into the informative. message (-DARCHIVE=/a/b/c) - so if /a/b/c is the wrong path the user can correct that property.

Will need to update the docs page after this of course.

macchiati · 2023-11-29T22:45:13Z

I don't see the problem with the default being false. Why can't we have the following? 1. no option 2. -DHAS_CLDR_ARCHIVE 3. -DHAS_CLDR_ARCHIVE=true Uses the CLDR ARCHIVE ==== 1. -DHAS_CLDR_ARCHIVE=false Skips the CLDR ARCHIVE

…

On Wed, Nov 29, 2023 at 2:37 PM Steven R. Loomis ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tools/cldr-code/src/test/java/org/unicode/cldr/util/TestCLDRPaths.java <#3215 (comment)>: > +package org.unicode.cldr.util; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +import java.io.File; +import org.junit.jupiter.api.Test; + +public class TestCLDRPaths { + + @test + void TestCanUseArchiveDirectory() { + if (!canUseArchiveDirectory()) { + // We print this warning as part of the unit tests. + System.err.println( + "WARNING: skipping using cldr-archive. Ideally, -DNO_CLDR_ARCHIVE=false and see <https://cldr.unicode.org/development/creating-the-archive>"); Except the default is false. How about: SKIP_CLDR_ARCHIVE=false as the default. You set it to true if you don't want to setup CLDR-archive. — Reply to this email directly, view it on GitHub <#3215 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMFHYEQNSJIGTI2AQBTYG62J7AVCNFSM6AAAAAA333SHU2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONJWGMZTGOJTGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

srl295 · 2023-11-30T00:11:14Z

false-by-default means that omitting any property (such as HAS_CLDR_ARCHIVE) means it has a false value, that's how other settings work. But i can make this one true by default.

- use braces, spotless

- also refactor in test code

srl295 · 2023-11-30T00:53:06Z

Example test run with default arguments (assumes -DHAS_CLDR_ARCHIVE=true)

[INFO] Running org.unicode.cldr.util.TestCLDRPaths
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 4.036 s - in org.unicode.cldr.util.TestCLDRPaths

Example test run with -DHAS_CLDR_ARCHIVE=false

[INFO] Running org.unicode.cldr.util.TestCLDRPaths
WARNING: -DHAS_CLDR_ARCHIVE=false, so skipping tests which use cldr-archive
See <https://cldr.unicode.org/development/creating-the-archive>
[WARNING] Tests run: 2, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.303 s - in org.unicode.cldr.util.TestCLDRPaths

Example test run where cldr-archive isn't setup properly:

[ERROR] org.unicode.cldr.util.TestCLDRPaths.TestCanUseArchiveDirectory  Time elapsed: 0.002 s  <<< ERROR!
java.lang.IllegalArgumentException: Could not read archive directory -DARCHIVE=/Users/srl295/src/cldr-archive
You must either:
 1) follow instructions at <https://cldr.unicode.org/development/creating-the-archive>, or
 2) skip cldr-archive tests by setting -DHAS_CLDR_ARCHIVE=false

macchiati · 2023-11-30T00:50:02Z

tools/cldr-code/src/main/java/org/unicode/cldr/tool/GenerateItemCounts.java

@@ -197,6 +197,7 @@ private static void checkBadAttributes(Relation<String, String> path2value2, Str

        SupplementalDataInfo supp = SUPPLEMENTAL_DATA_INFO;
        for (DtdType dtdType : DtdType.values()) {
+            if (dtdType.getStatus() != DtdType.DtdStatus.active) continue;


I think there's an Eclipse tool that will add {...} after every if statement. If you want to fix these (and others) that way, a separate PR would work.

Since were using spotless it would be better to set spotless to do this.

@macchiati there are 2,000+ cases of single-line if statements in the code. I couldn't find a spotless setting to fix this, though eclipse has one.

@macchiati @srl295 here is a PR with comments implying that Google Java Style does call for doing what Mark wants here, however google-java-format has a "constraint of only modifying whitespace": google/google-java-format#938

The implication seems to be that if we inserted brackets as a one-time operation using a different tool, then spotless configured for google-java-format would preserve the brackets, but exceptions could still arise in new code

srl295 requested review from macchiati and btangmu August 23, 2023 18:14

srl295 self-assigned this Aug 23, 2023

btangmu previously approved these changes Aug 23, 2023

View reviewed changes

macchiati previously approved these changes Aug 23, 2023

View reviewed changes

srl295 dismissed stale reviews from macchiati and btangmu via 4203997 August 23, 2023 23:43

srl295 force-pushed the cldr-16393/ci-cldr-archive branch from 0b2d39b to 4203997 Compare August 23, 2023 23:43

srl295 marked this pull request as ready for review August 23, 2023 23:50

srl295 force-pushed the cldr-16393/ci-cldr-archive branch from bbacbcc to 0b683a6 Compare August 30, 2023 22:52

srl295 added 5 commits November 29, 2023 11:41

CLDR-16393 assume cldr-archive is present for tests

bdd4931

- UNLESS -DNO_CLDR_ARCHIVE=true is set

CLDR-16393 reinstate some tests that depend on cldr-archive

1ed1b38

CLDR-16393 add a test for loading SDI

cde86d9

- also, reinstate the 'platform' DtdType as 'removed'

CLDR-16393 fix breakage with old DTDs

a5d79eb

- skip ldmlICU versions that are missing - ElementAttributeInfo needs to skip XML files that are missing

CLDR-16393 spotless

50d8877

CLDR-16393 merge

4bd5711

srl295 force-pushed the cldr-16393/ci-cldr-archive branch from 1fd4f05 to 4bd5711 Compare November 29, 2023 20:57

srl295 requested review from btangmu and macchiati November 29, 2023 21:04

macchiati requested changes Nov 29, 2023

View reviewed changes

srl295 added 2 commits November 29, 2023 18:34

CLDR-16393 code review comments

bb85440

- use braces, spotless

CLDR-16393 change the property to -DHAS_CLDR_ARCHIVE

50bf437

- also refactor in test code

srl295 added 2 commits November 29, 2023 18:53

CLDR-16393 update failure message

84a677c

CLDR-16393 spotless

d29b102

macchiati reviewed Nov 30, 2023

View reviewed changes

macchiati approved these changes Nov 30, 2023

View reviewed changes

srl295 merged commit 974b361 into unicode-org:main Nov 30, 2023
10 checks passed

srl295 deleted the cldr-16393/ci-cldr-archive branch November 30, 2023 02:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLDR-16393 assume cldr-archive is present for tests #3215

CLDR-16393 assume cldr-archive is present for tests #3215

srl295 commented Aug 23, 2023 •

edited

Loading

jira-pull-request-webhook bot commented Aug 23, 2023

srl295 commented Aug 24, 2023

srl295 commented Aug 26, 2023

jira-pull-request-webhook bot commented Aug 30, 2023

macchiati commented Aug 30, 2023

srl295 commented Aug 30, 2023

macchiati commented Aug 30, 2023

srl295 commented Aug 31, 2023

macchiati commented Aug 31, 2023 via email

macchiati commented Aug 31, 2023 via email

srl295 commented Aug 31, 2023

srl295 commented Aug 31, 2023

macchiati commented Aug 31, 2023

srl295 commented Aug 31, 2023

macchiati commented Aug 31, 2023 via email

jira-pull-request-webhook bot commented Nov 29, 2023

srl295 commented Nov 29, 2023

srl295 commented Nov 29, 2023

srl295 commented Nov 29, 2023 •

edited

Loading

srl295 commented Nov 29, 2023

macchiati left a comment

macchiati Nov 29, 2023

macchiati Nov 29, 2023

macchiati Nov 29, 2023 •

edited by srl295

Loading

srl295 Nov 30, 2023

macchiati commented Nov 29, 2023 via email

srl295 commented Nov 30, 2023

srl295 commented Nov 30, 2023

macchiati Nov 30, 2023

srl295 Nov 30, 2023

srl295 Nov 30, 2023

btangmu Dec 1, 2023

CLDR-16393 assume cldr-archive is present for tests #3215

CLDR-16393 assume cldr-archive is present for tests #3215

Conversation

srl295 commented Aug 23, 2023 • edited Loading

jira-pull-request-webhook bot commented Aug 23, 2023

srl295 commented Aug 24, 2023

srl295 commented Aug 26, 2023

jira-pull-request-webhook bot commented Aug 30, 2023

macchiati commented Aug 30, 2023

srl295 commented Aug 30, 2023

macchiati commented Aug 30, 2023

srl295 commented Aug 31, 2023

macchiati commented Aug 31, 2023 via email

macchiati commented Aug 31, 2023 via email

srl295 commented Aug 31, 2023

srl295 commented Aug 31, 2023

macchiati commented Aug 31, 2023

srl295 commented Aug 31, 2023

macchiati commented Aug 31, 2023 via email

jira-pull-request-webhook bot commented Nov 29, 2023

srl295 commented Nov 29, 2023

srl295 commented Nov 29, 2023

srl295 commented Nov 29, 2023 • edited Loading

srl295 commented Nov 29, 2023

macchiati left a comment

Choose a reason for hiding this comment

macchiati Nov 29, 2023

Choose a reason for hiding this comment

macchiati Nov 29, 2023

Choose a reason for hiding this comment

macchiati Nov 29, 2023 • edited by srl295 Loading

Choose a reason for hiding this comment

srl295 Nov 30, 2023

Choose a reason for hiding this comment

macchiati commented Nov 29, 2023 via email

srl295 commented Nov 30, 2023

srl295 commented Nov 30, 2023

macchiati Nov 30, 2023

Choose a reason for hiding this comment

srl295 Nov 30, 2023

Choose a reason for hiding this comment

srl295 Nov 30, 2023

Choose a reason for hiding this comment

btangmu Dec 1, 2023

Choose a reason for hiding this comment

srl295 commented Aug 23, 2023 •

edited

Loading

srl295 commented Nov 29, 2023 •

edited

Loading

macchiati Nov 29, 2023 •

edited by srl295

Loading