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

MWPW-143904-Unit test and small authoring update for mobile-app-banner #2126

Merged
merged 5 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions libs/blocks/mobile-app-banner/mobile-app-banner.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ function branchInit(header, key) {
export default async function init(el) {
const header = document.querySelector('.global-navigation');
if (!header) return;
const row = el.querySelector(':scope > div');
const product = row.textContent.trim().toLowerCase();
const classListArray = Array.from(el.classList);
const product = classListArray.find((token) => token.startsWith('product-')).split('-')[1];
Comment on lines +65 to +66
Copy link
Contributor

@mokimo mokimo Apr 10, 2024

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

Copy link
Contributor

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.

const key = await getKey(product);
if (key) branchInit(header, key);
}
70 changes: 70 additions & 0 deletions test/blocks/mobile-app-banner/mobile-app-banner.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
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' });
document.head.innerHTML = '<script/>'; // This block needs a script in head.
Ruchika4 marked this conversation as resolved.
Show resolved Hide resolved
});
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(100);
Ruchika4 marked this conversation as resolved.
Show resolved Hide resolved

const scriptTags = document.querySelectorAll('head > script');
const scriptSrcs = [];
scriptTags.forEach((scriptTag) => {
scriptSrcs.push(scriptTag.getAttribute('src'));
});
Comment on lines +31 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const scriptSrcs = [];
scriptTags.forEach((scriptTag) => {
scriptSrcs.push(scriptTag.getAttribute('src'));
});
const scriptSrcs = scriptTags.map(script => script.getAttribute('src'));

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(100);

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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
const scriptSrcs = [];
scriptTags.forEach((scriptTag) => {
if (scriptTag.getAttribute('src') !== null) scriptSrcs.push(scriptTag.getAttribute('src'));
});
const scriptSrcs = scriptTags.map(script => script.getAttribute('src'))

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(100);
window.dispatchEvent(new CustomEvent('adobePrivacy:PrivacyCustom')); // no init twice
Ruchika4 marked this conversation as resolved.
Show resolved Hide resolved
await delay(100);
Ruchika4 marked this conversation as resolved.
Show resolved Hide resolved
const scriptTags = document.querySelectorAll('head > script');
const scriptSrcs = [];
scriptTags.forEach((scriptTag) => {
Copy link
Contributor

Choose a reason for hiding this comment

The 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');
});
});
10 changes: 10 additions & 0 deletions test/blocks/mobile-app-banner/mocks/body.html
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>
8 changes: 8 additions & 0 deletions test/blocks/mobile-app-banner/mocks/branch-io-key.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"data": [
{
"product": "test",
"key": "key_test_eaNdoH8nTxeZXfOsgkELrjgpFrhm4q2m"
}
]
}
Loading