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

Minor simplification of LinkTest #566

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

HannesWell
Copy link
Member

@HannesWell HannesWell commented Jul 6, 2023

And investigate why the test is failing.

Fixes #561

@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

Test Results

       42 files  ±0         42 suites  ±0   48m 25s ⏱️ - 7m 32s
  3 754 tests ±0    3 750 ✔️ +1    4 💤 ±0  0 ±0 
11 265 runs  ±0  11 235 ✔️ +1  30 💤 ±0  0 ±0 

Results for commit 6c8adc9. ± Comparison against base commit c5416ba.

♻️ This comment has been updated with latest results.

@HannesWell
Copy link
Member Author

The offending link is probably on <help-URL>/help/topic/org.eclipse.platform.doc.isv/reference/extension-points/org_eclipse_help_index.html
My guess is that the failure of the LinkTest is caused by a space in some Extension-Point documentation in JDT.
Just created eclipse-jdt/eclipse.jdt.debug#278 to hopeful fix it.

The help-sites are

@HannesWell HannesWell force-pushed the simplifyLinkTest branch 4 times, most recently from 1622846 to 253e1a6 Compare July 8, 2023 08:10
@HannesWell
Copy link
Member Author

Even after eclipse-jdt/eclipse.jdt.debug#278 is merged and one I-build has passed the space in beginning is still found.
Stripping the string solves that problem and lets that test pass, but of course it would swallow similar problems in the future.

Does anybody know from which repo the IWorkbenchHelpSystem returned by PlatformUI.getWorkbench().getHelpSystem() is feed? Do I have to wait for another I-build, for the next milestone or even next release to get the updated version?

Copy link
Contributor

@Bananeweizen Bananeweizen left a comment

Choose a reason for hiding this comment

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

I suggest merging your change and not trying to find the original HTML document. AFAIK blanks around URIs are valid in HTML: https://www.w3.org/TR/2014/REC-html5-20141028/infrastructure.html#valid-url-potentially-surrounded-by-spaces

@Bananeweizen
Copy link
Contributor

If I get it right, the source containing the blank has already been fixed in #561.

@merks
Copy link
Contributor

merks commented Jul 9, 2023

If I get it right, the source containing the blank has already been fixed in #561.

Yes that's right. We did track down the offending files.

@HannesWell HannesWell changed the title Minor simplification of LinkTest Fix LinkTest failure due to leading whitespace and simplify code Jul 9, 2023
index = closeIndex;
for (String inputLine; (inputLine = in.readLine()) != null;) {
for (Matcher matcher = HREF.matcher(inputLine); matcher.find();) {
URI href = URI.create(matcher.group(1).replace(" ", "%20"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @HannesWell. I misread your code during first review. We still need to trim the regex result here to deal with URIs starting/ending with blanks.

Suggested change
URI href = URI.create(matcher.group(1).replace(" ", "%20"));
URI href = URI.create(matcher.group(1).strip().replace(" ", "%20"));

@HannesWell
Copy link
Member Author

I suggest merging your change and not trying to find the original HTML document. AFAIK blanks around URIs are valid in HTML: https://www.w3.org/TR/2014/REC-html5-20141028/infrastructure.html#valid-url-potentially-surrounded-by-spaces

Thanks Michael for pointing this out, but unfortunately the help still uses html-4 as mentioned in #561 (comment). 😞

But I wonder why my changes in the Extension-Point schema are not picked up in an I-build when building the doc?
It looks like the doc is build in the I-builds:
https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/92d943bcd4eeba3187e64fc4f154ab029e234364/eclipse.platform.common/bundles/org.eclipse.jdt.doc.isv/buildDoc.xml#L33

Therefore I assumed the change fix is now included, but I still se it in my latest I-build SDK.

@Bananeweizen
Copy link
Contributor

Thanks Michael for pointing this out, but unfortunately the help still uses html-4 as mentioned in #561 (comment). 😞

I'm absolutely not an expert on those standards, but looking at the differences between 4 and 5, the href attribute has no relevant changes: https://www.w3.org/TR/html5-diff/#changed-attributes. Therefore URIs starting/ending with blanks should be valid there, too.

Copy link
Contributor

@Bananeweizen Bananeweizen left a comment

Choose a reason for hiding this comment

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

Seems my knowledge of the standard is not sufficient enough (given the arguments in the other thread), so I'm removing my veto.

@HannesWell HannesWell changed the title Fix LinkTest failure due to leading whitespace and simplify code Minor simplification of LinkTest Jul 9, 2023
@HannesWell
Copy link
Member Author

Thanks Michael for pointing this out, but unfortunately the help still uses html-4 as mentioned in #561 (comment). 😞

I'm absolutely not an expert on those standards, but looking at the differences between 4 and 5, the href attribute has no relevant changes: https://www.w3.org/TR/html5-diff/#changed-attributes. Therefore URIs starting/ending with blanks should be valid there, too.

Me neither. But in doubt, I would prefer to be stricter in this case, just to be sure.
And with eclipse-platform/eclipse.platform.releng.aggregator#1209 the fix in the jdt-doc should be picked up in tonights I-build and the failures should be gone.

@HannesWell
Copy link
Member Author

Test is green now.

@HannesWell HannesWell merged commit eb82bed into eclipse-platform:master Jul 10, 2023
12 checks passed
@HannesWell HannesWell deleted the simplifyLinkTest branch July 10, 2023 06:16
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.

Failure of LinkTest.testAllLinks()
3 participants