-
Notifications
You must be signed in to change notification settings - Fork 753
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
#2872 update the AEM Mocks to 5.6.2 and made all tests working again #2875
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2875 +/- ##
=========================================
Coverage 87.15% 87.15%
Complexity 2692 2692
=========================================
Files 235 235
Lines 7188 7188
Branches 1100 1100
=========================================
Hits 6265 6265
Misses 365 365
Partials 558 558 ☔ View full report in Codecov by Sentry. |
bundles/core/src/test/java/com/adobe/cq/wcm/core/components/internal/link/LinkTestUtils.java
Dismissed
Show dismissed
Hide dismissed
Quality Gate passedIssues Measures |
class LinkImplTest { | ||
|
||
private static final String URL = "/url.html"; | ||
|
||
private final AemContext context = CoreComponentTestContext.newAemContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we avoid passing the context in all assertValidLinks
? I see the context is instantiated here, maybe we can do that in LinkTestUtils?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertValidLink
is also used by other test classes; for example in LInkManagerTest
I see that the context is also used in other ways. I haven't investigated if these tests would also work with 2 different instances of the AemContext, but I also don't think that it's worth the effort.
@@ -73,7 +73,7 @@ void testResourceExternalLink() { | |||
context.currentResource(linkResource); | |||
Link link = getUnderTest().get(linkResource).build(); | |||
|
|||
assertValidLink(link, "http://myhost"); | |||
assertValidLink(link, "http://myhost",context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacing is off here (and in other places too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, fixed it.
@@ -117,7 +117,7 @@ void testVanityConfig() { | |||
DefaultPathProcessor underTest = context.registerInjectActivateService(new DefaultPathProcessor(), ImmutableMap.of( | |||
"vanityConfig", "shouldBeDefault")); | |||
assertEquals("/content/site1/en.html", underTest.map(page.getPath() + HTML_EXTENSION, context.request())); | |||
assertEquals("https://example.org/content/links/site1/en.html", underTest.externalize(page.getPath() + HTML_EXTENSION, context.request())); | |||
assertEquals("https://example.org/content/site1/en.html", underTest.externalize(page.getPath() + HTML_EXTENSION, context.request())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a missing /links
which puzzles me, who is removing it and why? Is this a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is caused by a change in the AEM Mocks (wcm-io/io.wcm.testing.aem-mock#44), which has been added in AEM Mocks 5.5.4
In order to make that update go I had to change testcases, as the MockExternalizer was changed (see https://wcm.io/testing/aem-mock/changes-report.html#a5.5.4).