8000 Table flaky/failing tests fix by DKos95 · Pull Request #4458 · adobecom/milo · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Table flaky/failing tests fix #4458

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 5, 2025
Merged

Table flaky/failing tests fix #4458

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
43d2411
Table flaky/failing tests fix
DKos95 Jun 24, 2025
08490a9
Table test beforeeach logic update
DKos95 Jun 24, 2025
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
13 changes: 11 additions & 2 deletions test/blocks/table/table.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ setConfig(conf);
const config = getConfig();

describe('table and tablemetadata', () => {
beforeEach(() => {
before(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

To me this looks like it builds up test-state and is an anti-pattern. If we depend on whatever one test has done - it's likely wrong.
Maybe we should just wipe the innerHTML and recreate the markup on each run - to properly isolate each test?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should stay a beforeEach and l7 should be moved into the beforeEach block

Copy link
Contributor
@mokimo mokimo Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

PS: If the tests already been written in a way, that they depend on whatever the test before has done - Good luck 😆
(Might not be worth the hassle in this case to do a refactoring of the whole file, that can be pretty time consuming)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with beforeEach here is that it initializes every table before each test and if we don't have a check in the table.js if the events have been added then it will add them again and cause bad tests not because of the tests but because of table.js attaching a listener again, so either we add the check in table.js to not attach listeners if already attached or change how we do this specific test like i did in this commit.

PS: moving l7 to beforeEach won't help 😅

Copy link
Contributor
@mokimo mokimo Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What if there are multiple table blocks on a page - wouldn't that also lead to the table block attaching multiple listeners? Maybe the tests hint at a (real) issue here actually 😁
Might be good to check, otherwise I think it's not worth spending too much time on the unit tests unless you have a lot of free time on your hand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried adding 2 tables on 1 page and added a console log but it only attached 2 times https://table-flaky-tests-fix--milo--adobecom.aem.page/drafts/dusan/check-multiple-tables.

const tables = document.querySelectorAll('.table');
tables.forEach((t) => init(t));
window.dispatchEvent(new Event(MILO_EVENTS.DEFERRED));
Expand Down Expand Up @@ -114,12 +114,21 @@ describe('table and tablemetadata', () => {
expect(tooltipHeading.childNodes.length).to.equal(2);
expect(tooltipHeading.querySelector('.milo-tooltip, .icon-tooltip')).to.exist;
});
});

describe('mobile aria-label test setup', () => {
beforeEach(() => {
const tables = document.querySelectorAll('.table');
tables.forEach((t) => init(t));
window.dispatchEvent(new Event(MILO_EVENTS.DEFERRED));
});

it('should apply aria-label to all selects within .filters on mobile', async () => {
window.innerWidth = 375;
window.dispatchEvent(new Event('resize'));
const filters = await waitForElement('.filters');
const selectElements = filters.querySelectorAll('select');
const ariaLabel = await replaceKey('choose-table-column', config);
const selectElements = document.querySelectorAll('.filters select');

selectElements.forEach((selectElement) => {
expect(selectElement.getAttribute('aria-label')).to.equal(ariaLabel);
Expand Down
Loading
0