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

Ruchika4 · 2024-04-09T21:12:43Z

Small authoring update for mobile-app-banner
Unit tests for mobile-app-banner

Test URLs:

CC

Authoring documentation:
https://main--milo--adobecom.hlx.page/docs/authoring/blocks/mobile-app-banner

aem-code-sync · 2024-04-09T21:12:46Z

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

Re-run PSI checks
Re-sync branch

Commits

1941abb ✅ (latest)
0850d61 ✅
cbc5978 ✅
1e0bae9 ✅
e32dea5 ✅

aem-code-sync · 2024-04-09T21:13:04Z

Page	Scores	Audits	Google
/drafts/ruchika/branch-io/branch-io-page11?martech=off

codecov · 2024-04-09T21:17:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.38%. Comparing base (be31bf0) to head (1941abb).
Report is 23 commits behind head on stage.

Additional details and impacted files

@@            Coverage Diff             @@
##            stage    #2126      +/-   ##
==========================================
+ Coverage   96.36%   96.38%   +0.02%     
==========================================
  Files         165      166       +1     
  Lines       43523    43630     +107     
==========================================
+ Hits        41941    42054     +113     
+ Misses       1582     1576       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

test/blocks/mobile-app-banner/mobile-app-banner.test.js

mokimo

Some smaller code sanity and efficiency comments. Otherwise looks good, thanks for following up on those unit tests!

test/blocks/mobile-app-banner/mobile-app-banner.test.js

mokimo · 2024-04-10T11:28:20Z

test/blocks/mobile-app-banner/mobile-app-banner.test.js

+    const scriptSrcs = [];
+    scriptTags.forEach((scriptTag) => {
+      scriptSrcs.push(scriptTag.getAttribute('src'));
+    });


Suggested change

const scriptSrcs = [];

scriptTags.forEach((scriptTag) => {

scriptSrcs.push(scriptTag.getAttribute('src'));

});

const scriptSrcs = scriptTags.map(script => script.getAttribute('src'));

mokimo · 2024-04-10T11:30:04Z

test/blocks/mobile-app-banner/mobile-app-banner.test.js

+    const scriptSrcs = [];
+    scriptTags.forEach((scriptTag) => {
+      if (scriptTag.getAttribute('src') !== null) scriptSrcs.push(scriptTag.getAttribute('src'));
+    });


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'))

test/blocks/mobile-app-banner/mobile-app-banner.test.js

mokimo · 2024-04-10T11:34:58Z

test/blocks/mobile-app-banner/mobile-app-banner.test.js

+    await delay(100);
+    const scriptTags = document.querySelectorAll('head > script');
+    const scriptSrcs = [];
+    scriptTags.forEach((scriptTag) => {


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');

mokimo · 2024-04-10T11:37:18Z

libs/blocks/mobile-app-banner/mobile-app-banner.js

+  const classListArray = Array.from(el.classList);
+  const product = classListArray.find((token) => token.startsWith('product-')).split('-')[1];


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

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.

overmyheadandbody

Ok overall, but @mokimo has some good points that should be considered.

overmyheadandbody · 2024-04-10T17:36:42Z

libs/blocks/mobile-app-banner/mobile-app-banner.js

+  const classListArray = Array.from(el.classList);
+  const product = classListArray.find((token) => token.startsWith('product-')).split('-')[1];


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.

test/blocks/mobile-app-banner/mobile-app-banner.test.js

Ruchika4 · 2024-04-11T15:43:07Z

Hi @spadmasa , could you please verify if the authoring update done in this PR is working as expected and block is rendering. Other issues that we are facing with android device will most likely be handled in branch dashboard or I'll be raising a separate PR to get it merged in milo.
This is a new block so there will be no impact on any production page.

spadmasa · 2024-04-11T15:54:34Z

Verified the changes in the below feature branch urls in mobile (iphone and android) and Ipad the banner is seen loading
https://branch-latest--milo--adobecom.hlx.page/drafts/ruchika/branch-io/branch-io-page11?martech=off

https://main--cc--adobecom.hlx.page/drafts/ruchika/mobile-app-banner1?milolibs=branch-latest&martech=off

https://branch-latest--milo--adobecom.hlx.page/kr/drafts/ruchika/branch-io/branch-io-page12

Ruchika4 added 2 commits April 8, 2024 21:52

Update-authoring

e32dea5

add unit test

1e0bae9

Ruchika4 added needs-verification PR requires E2E testing by a reviewer high priority Why is this a high priority? Blocker? Critical? Dependency? labels Apr 9, 2024

Ruchika4 requested review from Blainegunn, mokimo, JasonHowellSlavin and narcis-radu April 9, 2024 21:15

Update mobile-app-banner.test.js

cbc5978

aem-code-sync bot temporarily deployed to branch-latest April 9, 2024 23:28 Inactive

Ruchika4 requested a review from TsayAdobe April 9, 2024 23:28

github-actions bot reviewed Apr 9, 2024

View reviewed changes

test/blocks/mobile-app-banner/mobile-app-banner.test.js Outdated Show resolved Hide resolved

lint fix

0850d61

aem-code-sync bot temporarily deployed to branch-latest April 9, 2024 23:29 Inactive

Ruchika4 assigned spadmasa Apr 10, 2024

mokimo approved these changes Apr 10, 2024

View reviewed changes

mokimo reviewed Apr 10, 2024

View reviewed changes

overmyheadandbody approved these changes Apr 11, 2024

View reviewed changes

incorporate review comments

1941abb

spadmasa added verified PR has been E2E tested by a reviewer and removed needs-verification PR requires E2E testing by a reviewer labels Apr 11, 2024

Ruchika4 added the Ready for Stage label Apr 11, 2024

Blainegunn merged commit 6453307 into stage Apr 11, 2024
17 checks passed

Blainegunn deleted the branch-latest branch April 11, 2024 19:36

Blainegunn mentioned this pull request Apr 11, 2024

[Release] Stage to Main #2138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Ruchika4 commented Apr 9, 2024

aem-code-sync bot commented Apr 9, 2024 •

edited

Loading

aem-code-sync bot commented Apr 9, 2024 •

edited

Loading

codecov bot commented Apr 9, 2024 •

edited

Loading

mokimo left a comment

mokimo Apr 10, 2024

mokimo Apr 10, 2024

mokimo Apr 10, 2024

mokimo Apr 10, 2024 •

edited

Loading

overmyheadandbody Apr 10, 2024

overmyheadandbody left a comment

overmyheadandbody Apr 10, 2024

Ruchika4 commented Apr 11, 2024

spadmasa commented Apr 11, 2024

		const classListArray = Array.from(el.classList);
		const product = classListArray.find((token) => token.startsWith('product-')).split('-')[1];

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

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

Conversation

Ruchika4 commented Apr 9, 2024

aem-code-sync bot commented Apr 9, 2024 • edited Loading

aem-code-sync bot commented Apr 9, 2024 • edited Loading

codecov bot commented Apr 9, 2024 • edited Loading

Codecov Report

mokimo left a comment

Choose a reason for hiding this comment

mokimo Apr 10, 2024

Choose a reason for hiding this comment

mokimo Apr 10, 2024

Choose a reason for hiding this comment

mokimo Apr 10, 2024

Choose a reason for hiding this comment

mokimo Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

overmyheadandbody Apr 10, 2024

Choose a reason for hiding this comment

overmyheadandbody left a comment

Choose a reason for hiding this comment

overmyheadandbody Apr 10, 2024

Choose a reason for hiding this comment

Ruchika4 commented Apr 11, 2024

spadmasa commented Apr 11, 2024

aem-code-sync bot commented Apr 9, 2024 •

edited

Loading

aem-code-sync bot commented Apr 9, 2024 •

edited

Loading

codecov bot commented Apr 9, 2024 •

edited

Loading

mokimo Apr 10, 2024 •

edited

Loading