You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:46:57 UTC
[GitHub] [sling-org-apache-sling-resourceresolver] bdelacretaz commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions
bdelacretaz commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r649806943
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -370,26 +381,31 @@ public void test_vanity_path_updates() throws Exception {
@Test
public void test_vanity_path_registration_include_exclude() throws IOException {
final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"};
- final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"};
final List<Resource> resources = new ArrayList<>();
for(final String val : validPaths) {
resources.add(getVanityPathResource(val));
}
- for(final String val : invalidPaths) {
- resources.add(getVanityPathResource(val));
- }
-
-
- when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer<Iterator<Resource>>() {
-
- @Override
- public Iterator<Resource> answer(InvocationOnMock invocation) throws Throwable {
- if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) {
- return resources.iterator();
- } else {
- return Collections.<Resource> emptySet().iterator();
- }
+ when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
Review comment:
Although you are checking that the query is what you expect, unless I missed something there's no test that validates the query syntax? I understand this is similar to the existing tests but I think it would be better to run that query against a content repository provided by Sling JCR Mocks and validate the results.
I don't know how much time you have for that - this way of testing probably works for now but IMHO running the query would create a test that's easier to maintain and understand. Maybe factor out the query generation to its own package-local helper class to avoid issues with private methods.
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -370,26 +381,31 @@ public void test_vanity_path_updates() throws Exception {
@Test
public void test_vanity_path_registration_include_exclude() throws IOException {
final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"};
- final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"};
final List<Resource> resources = new ArrayList<>();
for(final String val : validPaths) {
resources.add(getVanityPathResource(val));
}
- for(final String val : invalidPaths) {
- resources.add(getVanityPathResource(val));
- }
-
-
- when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer<Iterator<Resource>>() {
-
- @Override
- public Iterator<Resource> answer(InvocationOnMock invocation) throws Throwable {
- if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) {
- return resources.iterator();
- } else {
- return Collections.<Resource> emptySet().iterator();
- }
+ when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
Review comment:
If other reviewers agree to merge this with the tests as is I will not object. Maybe just add a comment to the test explaining that actually running the query would be better?
##########
File path: src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -370,26 +381,31 @@ public void test_vanity_path_updates() throws Exception {
@Test
public void test_vanity_path_registration_include_exclude() throws IOException {
final String[] validPaths = {"/libs/somewhere", "/libs/a/b", "/foo/a", "/baa/a"};
- final String[] invalidPaths = {"/libs/denied/a", "/libs/denied/b/c", "/nowhere"};
final List<Resource> resources = new ArrayList<>();
for(final String val : validPaths) {
resources.add(getVanityPathResource(val));
}
- for(final String val : invalidPaths) {
- resources.add(getVanityPathResource(val));
- }
-
-
- when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer(new Answer<Iterator<Resource>>() {
-
- @Override
- public Iterator<Resource> answer(InvocationOnMock invocation) throws Throwable {
- if (invocation.getArguments()[0].toString().contains("sling:vanityPath")) {
- return resources.iterator();
- } else {
- return Collections.<Resource> emptySet().iterator();
- }
+ when(resourceResolver.findResources(anyString(), eq("sql"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
Review comment:
The [jcr/repoinit/ExecutionOrderTest](https://github.com/apache/sling-org-apache-sling-jcr-repoinit/blob/master/src/test/java/org/apache/sling/jcr/repoinit/ExecutionOrderTest.java) is an example using SlingContext. I agree with Robert that such things might be better tested in integration tests, and as mentioned I won't object to committing the code without a query validation test.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org