-
Notifications
You must be signed in to change notification settings - Fork 177
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
MWPW-143904-Unit test and small authoring update for mobile-app-banner #2126
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,67 @@ | ||||||||||||
import { readFile } from '@web/test-runner-commands'; | ||||||||||||
import { expect } from '@esm-bundle/chai'; | ||||||||||||
import sinon from 'sinon'; | ||||||||||||
import { delay } from '../../helpers/waitfor.js'; | ||||||||||||
|
||||||||||||
const { setConfig } = await import('../../../libs/utils/utils.js'); | ||||||||||||
|
||||||||||||
const mockConfig = { contentRoot: '/test/blocks/mobile-app-banner/mocks' }; | ||||||||||||
setConfig(mockConfig); | ||||||||||||
|
||||||||||||
describe('mobile-app-banner', () => { | ||||||||||||
beforeEach(async () => { | ||||||||||||
document.body.innerHTML = await readFile({ path: './mocks/body.html' }); | ||||||||||||
}); | ||||||||||||
afterEach(() => { | ||||||||||||
sinon.restore(); | ||||||||||||
}); | ||||||||||||
|
||||||||||||
it('should not call branch init if no branch-io-key.json', async () => { | ||||||||||||
sinon.stub(window, 'fetch'); | ||||||||||||
const res = new window.Response('Not found', { status: 404 }); | ||||||||||||
window.fetch.returns(Promise.resolve(res)); | ||||||||||||
|
||||||||||||
const module = await import('../../../libs/blocks/mobile-app-banner/mobile-app-banner.js'); | ||||||||||||
const banner = document.body.querySelector('.mobile-app-banner.product-test'); | ||||||||||||
await module.default(banner); | ||||||||||||
window.dispatchEvent(new CustomEvent('adobePrivacy:PrivacyConsent')); | ||||||||||||
await delay(0); | ||||||||||||
|
||||||||||||
const scriptTags = document.querySelectorAll('head > script'); | ||||||||||||
const scriptSrcs = []; | ||||||||||||
scriptTags.forEach((scriptTag) => { | ||||||||||||
scriptSrcs.push(scriptTag.getAttribute('src')); | ||||||||||||
}); | ||||||||||||
Comment on lines
+31
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
expect(scriptSrcs).to.not.include('https://cdn.branch.io/branch-latest.min.js'); | ||||||||||||
}); | ||||||||||||
|
||||||||||||
it('should not call branch init if product not found in branch-io-key file', async () => { | ||||||||||||
const module = await import('../../../libs/blocks/mobile-app-banner/mobile-app-banner.js'); | ||||||||||||
const banner = document.body.querySelector('.mobile-app-banner.product-test1'); | ||||||||||||
await module.default(banner); | ||||||||||||
window.dispatchEvent(new CustomEvent('adobePrivacy:PrivacyConsent')); | ||||||||||||
await delay(0); | ||||||||||||
|
||||||||||||
const scriptTags = document.querySelectorAll('head > script'); | ||||||||||||
const scriptSrcs = []; | ||||||||||||
scriptTags.forEach((scriptTag) => { | ||||||||||||
if (scriptTag.getAttribute('src') !== null) scriptSrcs.push(scriptTag.getAttribute('src')); | ||||||||||||
}); | ||||||||||||
Comment on lines
+46
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you check the whole content of the array - you don't need to check if the src is null
Suggested change
|
||||||||||||
expect(scriptSrcs).to.not.include('https://cdn.branch.io/branch-latest.min.js'); | ||||||||||||
}); | ||||||||||||
|
||||||||||||
it('should init by adding branchio script', async () => { | ||||||||||||
window.adobePrivacy = { hasUserProvidedConsent: () => true }; | ||||||||||||
const module = await import('../../../libs/blocks/mobile-app-banner/mobile-app-banner.js'); | ||||||||||||
const banner = document.body.querySelector('.mobile-app-banner.product-test'); | ||||||||||||
await module.default(banner); | ||||||||||||
window.dispatchEvent(new CustomEvent('adobePrivacy:PrivacyConsent')); | ||||||||||||
await delay(0); | ||||||||||||
const scriptTags = document.querySelectorAll('head > script'); | ||||||||||||
const scriptSrcs = []; | ||||||||||||
scriptTags.forEach((scriptTag) => { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same suggestion as above to simplify. In fact since you re-use this three times you could also const getScripts = () => [...document.querySelectorAll('head > script')].map(el => el.getAttribute('src')); And then you can simplify your expect expect(getScripts()).to.include('https://cdn.branch.io/branch-latest.min.js'); |
||||||||||||
if (scriptTag.getAttribute('src') !== null) scriptSrcs.push(scriptTag.getAttribute('src')); | ||||||||||||
}); | ||||||||||||
expect(scriptSrcs).to.include('https://cdn.branch.io/branch-latest.min.js'); | ||||||||||||
}); | ||||||||||||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
<header class="global-navigation" daa-im="true" daa-lh="gnav|milo"><div class="feds-curtain"></div><div class="feds-topnav-wrapper"> | ||
</header> | ||
<div>Hello World</div> | ||
<div> | ||
<div class="mobile-app-banner product-test"> | ||
</div> | ||
<div> | ||
<div class="mobile-app-banner product-test1"> | ||
</div> | ||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"data": [ | ||
{ | ||
"product": "test", | ||
"key": "key_test_eaNdoH8nTxeZXfOsgkELrjgpFrhm4q2m" | ||
} | ||
] | ||
} |
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.
I'm personally not a huge fan of mixing code changes with unit tests - since it requires regression testing - rather than saying "Those are only unit tests, it's zero-impact and doesn't need any testing since at worst we have broken tests (and not a CSO)"
It's completely fine to do, but that's my general rationale behind it
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.
Agree. The process might be adapted to streamline delivery of zero-impact PRs to prod, adding code to something that would otherwise be zero-impact will slow down the PR in the future. Of course, it can be that unit tests uncover some issues that need to be addressed, then it's totally fine to group them together.
For this particular PR, it shouldn't be an issue though.