You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "massdosage (via GitHub)" <gi...@apache.org> on 2024/08/23 12:17:11 UTC
[PR] Use platform-agnostic file separator to fix test failing on Windows [httpcomponents-client]
massdosage opened a new pull request, #571:
URL: https://github.com/apache/httpcomponents-client/pull/571
As discussed on the dev mailing list, this should fix the failing `TestPublicSuffixMatcher` tests on Windows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Use platform-agnostic file separator to fix test failing on Windows [httpcomponents-client]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #571:
URL: https://github.com/apache/httpcomponents-client/pull/571#issuecomment-2306993341
Hello @massdosage
FWIW, `Paths.get("mozilla", "public-suffix-list.txt").toString()` would be better here but not really needed unless `Path`s where used in the test. The `"/"` does not matter in Java on or off Windows, Java normalizes the path for the platform.
The test set up is also problematic because it leaks a file handle. We should be using a try-with-resource here:
```java
// Create a matcher using a custom crafted public suffix list file
try (InputStream in = classLoader.getResourceAsStream(SOURCE_FILE)) {
Assertions.assertNotNull(in);
final List<PublicSuffixList> lists = PublicSuffixListParser.INSTANCE.parseByType(new InputStreamReader(in, StandardCharsets.UTF_8));
matcher = new PublicSuffixMatcher(lists);
}
```
I fixed the above resource leak in git master.
The issue remains with the odd test failure on my end though :-(
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Use platform-agnostic file separator to fix test failing on Windows [httpcomponents-client]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #571:
URL: https://github.com/apache/httpcomponents-client/pull/571#issuecomment-2307184428
Ah, this PR looks like a partial duplicate of https://github.com/apache/httpcomponents-client/pull/574 except that #574 preserves the ability to refresh the DAT file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Use platform-agnostic file separator to fix test failing on Windows [httpcomponents-client]
Posted by "massdosage (via GitHub)" <gi...@apache.org>.
massdosage commented on PR #571:
URL: https://github.com/apache/httpcomponents-client/pull/571#issuecomment-2307037421
OK, the `SOURCE_FILE` code was already there before any of my changes so I purposefully didn't touch it but the try-with-resources you suggest looks good. Even if this PR doesn't fix the issue at least it will give a better error messages if the mozilla file wasn't downloaded.
I'm happy to make any changes you suggest here though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Use platform-agnostic file separator to fix test failing on Windows [httpcomponents-client]
Posted by "massdosage (via GitHub)" <gi...@apache.org>.
massdosage commented on code in PR #571:
URL: https://github.com/apache/httpcomponents-client/pull/571#discussion_r1729008351
##########
httpclient5/src/test/java/org/apache/hc/client5/http/psl/TestPublicSuffixMatcher.java:
##########
@@ -40,8 +42,7 @@
class TestPublicSuffixMatcher {
private static final String SOURCE_FILE = "suffixlistmatcher.txt";
- private static final String PUBLIC_SUFFIX_LIST_FILE = "mozilla/public-suffix-list.txt";
Review Comment:
I've made that change here now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Include Mozilla public suffix list file directly [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c closed pull request #571: Include Mozilla public suffix list file directly
URL: https://github.com/apache/httpcomponents-client/pull/571
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Include Mozilla public suffix list file directly [httpcomponents-client]
Posted by "massdosage (via GitHub)" <gi...@apache.org>.
massdosage commented on PR #571:
URL: https://github.com/apache/httpcomponents-client/pull/571#issuecomment-2307191939
Feel free to close this one and use #574 instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Use platform-agnostic file separator to fix test failing on Windows [httpcomponents-client]
Posted by "massdosage (via GitHub)" <gi...@apache.org>.
massdosage commented on code in PR #571:
URL: https://github.com/apache/httpcomponents-client/pull/571#discussion_r1728971628
##########
httpclient5/src/test/java/org/apache/hc/client5/http/psl/TestPublicSuffixMatcher.java:
##########
@@ -40,8 +42,7 @@
class TestPublicSuffixMatcher {
private static final String SOURCE_FILE = "suffixlistmatcher.txt";
- private static final String PUBLIC_SUFFIX_LIST_FILE = "mozilla/public-suffix-list.txt";
Review Comment:
OK, yeah, it's been ages since I touched Windows so I wasn't sure. In much earlier versions of Java this would have been an issue. If we instead prefer including the mozilla file in this repo like we do with `SOURCE_FILE` I can adjust this PR to do that instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Use platform-agnostic file separator to fix test failing on Windows [httpcomponents-client]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #571:
URL: https://github.com/apache/httpcomponents-client/pull/571#discussion_r1728960187
##########
httpclient5/src/test/java/org/apache/hc/client5/http/psl/TestPublicSuffixMatcher.java:
##########
@@ -40,8 +42,7 @@
class TestPublicSuffixMatcher {
private static final String SOURCE_FILE = "suffixlistmatcher.txt";
- private static final String PUBLIC_SUFFIX_LIST_FILE = "mozilla/public-suffix-list.txt";
Review Comment:
@massdosage That notation should work on all platforms. I am quite sure of it. This is not the right fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org