Skip to content

Convert dom.ids.Test to JUnit - #103

Merged
elharo merged 1 commit into
mainfrom
convert-ids-to-junit
Jul 10, 2026
Merged

Convert dom.ids.Test to JUnit#103
elharo merged 1 commit into
mainfrom
convert-ids-to-junit

Conversation

@elharo

@elharo elharo commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Convert the dom.ids.Test test harness from a main()-driven to JUnit 3.\

  • Extends TestCase
  • Replaces broken Assertion.verify/Assertion.equals with proper JUnit assertions
  • Registered in batchtest and old java task removed in build.xml

@elharo
elharo force-pushed the convert-ids-to-junit branch from b1ce8c7 to 04cd723 Compare July 7, 2026 22:04
elharo
elharo commented Jul 8, 2026
View reviewed changes
Comment thread tests/dom/ids/Test.java Outdated
el2 = doc.getElementById("my.worker");
Assertion.verify(el2 == el);
assertTrue("el2 == el", el2 == el);

@elharo elharo Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertEquals with no message

Comment thread tests/dom/ids/Test.java Outdated

el2 = doc.getElementById("one.worker");
Assertion.verify(el2 == null);
assertTrue("el2 == null", el2 == null);

@elharo elharo Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertNull with no message

Comment thread tests/dom/ids/Test.java Outdated
el.removeAttribute("id");
el2 = doc.getElementById("my.worker");
Assertion.verify(el2 == null);
assertTrue("el2 == null", el2 == null);

@elharo elharo Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertNull with no message

Comment thread tests/dom/ids/Test.java Outdated
elem = doc.getElementById("default.id");
Assertion.verify(elem !=null, "elem by id 'default.id'");

assertTrue("elem by id 'default.id'", elem != null);

@elharo elharo Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertNotNull with no message

Comment thread tests/dom/ids/Test.java Outdated
elem = doc.getElementById("id02");
Assertion.verify(elem ==null, "elem by id '02'");
assertTrue("elem by id '02'", elem == null);

@elharo elharo Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertNull with no message

Comment thread tests/dom/ids/Test.java Outdated
elem = doc.getElementById("eb0009");
Assertion.verify(elem !=null, "elem by id 'eb0009'");
assertTrue("elem by id 'eb0009'", elem != null);

@elharo elharo Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general here and following use assertEquals and assertNull and assertNotNull and assertSame as appropriate rather than justa ssertTrrue

@elharo elharo changed the title [XERCESJ-1796] Convert dom.ids.Test to JUnit 3 Convert dom.ids.Test to JUnit 3 Jul 8, 2026
@elharo
elharo force-pushed the convert-ids-to-junit branch 2 times, most recently from 46e5d55 to 7f7fae5 Compare July 8, 2026 12:02
garydgregory
garydgregory reviewed Jul 8, 2026
View reviewed changes

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's seems odd to add Junit 3 code here. Why not use the latest version that supports Java 8? Junit 5 IIRC.

@elharo

elharo commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Because we're already using JUnit 3. For the moment I'm focusing on the minimum changes needed to get the tests to run. (Right now they aren't.) If more code reviewer attention becomes available, then there are a lot of improvements that could be made, though frankly upgrading JUnit is very far down the list.

@garydgregory

garydgregory commented Jul 8, 2026

Copy link
Copy Markdown
Member

Thank you for the clarification. All builds are red tbough.

@mrglavas

mrglavas commented Jul 8, 2026

Copy link
Copy Markdown

Thank you for the clarification. All builds are red tbough.

Looks like the same thing being flagged on each build:

 [xjavac] /home/runner/work/xerces-j/xerces-j/build/tests/dom/ids/Test.java:104: error: variable doc might not have been initialized
   [xjavac]         Element el = doc.getElementById("one.worker");
   [xjavac]

mrglavas
mrglavas reviewed Jul 8, 2026
View reviewed changes
Comment thread tests/dom/ids/Test.java Outdated
parser.setFeature(DEFERRED_DOM_FEATURE_ID, DEFAULT_DEFERRED_DOM);
}

Document doc;

@mrglavas mrglavas Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps do this to resolve the build failure:

Suggested change
Document doc;
Document doc = null;

@elharo
elharo marked this pull request as draft July 8, 2026 18:01
@elharo
elharo force-pushed the convert-ids-to-junit branch from 7f7fae5 to ef3f325 Compare July 8, 2026 18:36
@elharo

elharo commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

fixed

PR #100 (range): Build failed on XML parse error in build.xml:919. The <java fork="yes" for dom.range.Test was
orphaned when the classname through lines were removed. Fixed — removed the dangling line. All 7 range
tests pass.

 PR #103 (ids): Build failed on compilation error: variable doc might not have been initialized at Test.java:104.   
 The try/catch with fail() doesn't convince the compiler that doc is always initialized. Fixed — changed Document   
 doc; to Document doc = null;.                                                                                      
                                                                                                                    
 PR #107 (testns): Test fails on assertTrue("Output should contain xx:attr", output.contains("xx:attr")) at         
 TestNS.java:116. The xerces LSSerializer doesn't output the qualified name xx:attr — it generates a different      
 prefix. This is a pre-existing bug in the serializer: the original main()-driven test printed output but never     
 validated it, so this failure was invisible. Left alone per instructions.

@elharo
elharo marked this pull request as ready for review July 8, 2026 18:40
@elharo elharo changed the title Convert dom.ids.Test to JUnit 3 Convert dom.ids.Test to JUnit Jul 8, 2026
@elharo

elharo commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Build passing now

garydgregory
garydgregory approved these changes Jul 9, 2026
View reviewed changes

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@elharo
elharo merged commit 435835c into main Jul 10, 2026
5 checks passed
@elharo
elharo deleted the convert-ids-to-junit branch July 10, 2026 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrglavas mrglavas mrglavas left review comments
@garydgregory garydgregory garydgregory approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@elharo @garydgregory @mrglavas