Skip to content

Conversation

LidiyaGeorgieva
Copy link
Contributor

JIRA: BGSOFUIRODOPI-3540

@LidiyaGeorgieva LidiyaGeorgieva requested a review from a team October 1, 2025 15:19
Copy link

@Stoev Stoev left a comment

Choose a reason for hiding this comment

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

Looks good.

cy.get("[ui5-busy-indicator]").invoke("removeAttr", "active");

// Wait for the original delay period
setTimeout(() => {}, 1300);
Copy link
Contributor

Choose a reason for hiding this comment

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

This setTimeout is not awaited by anything.
I recommend to change the entire test like this

cy.mount(
			<BusyIndicator delay={1200}>
				<div>Content</div>
			</BusyIndicator>
		);

		// Activate the busy indicator
		cy.get("[ui5-busy-indicator]").invoke("attr", "active", "");
		
		// Verify it's not busy yet (delay hasn't passed)
		cy.get("[ui5-busy-indicator]")
			.shadow()
			.find(".ui5-busy-indicator-busy-area")
			.should("not.exist");

		cy.get<BusyIndicator>("[ui5-busy-indicator]").should(($el) => {
			const timeoutId = $el[0]._busyTimeoutId;
			expect(timeoutId).to.exist;
		}).then(($el) => {
			cy.wrap($el[0]._busyTimeoutId).as("timeoutId");
		});

		const clearTimeoutSpy = cy.spy(clearTimeout);
		// Deactivate before delay passes - this should trigger the timeout cleanup
		cy.get("[ui5-busy-indicator]").invoke("removeAttr", "active");

		cy.get("[ui5-busy-indicator]")
			.shadow()
			.find(".ui5-busy-indicator-busy-area")
			.should("not.exist");

		cy.get("@timeoutId")
			.should((timeoutId)	=> {
				// expect(timeoutId).to.exist;
				expect(clearTimeoutSpy).to.have.been.calledWith(timeoutId);
			});

		cy.get("[ui5-busy-indicator]")
			.invoke("prop", "_isBusy")
			.should("eq", false);

There seems to be a bug, because clearTimeout is not called.

cy.get("[ui5-busy-indicator]").invoke("removeAttr", "active");

// Wait longer than the original delay to ensure timeout was cleared
setTimeout(() => {}, 600);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as in the test above.

Comment on lines +339 to +368
it("should trigger timeout cleanup when deactivated with pending timeout", () => {
cy.mount(
<BusyIndicator delay={1000}>
<div>Content</div>
</BusyIndicator>
);

// Test the specific scenario that hits the timeout cleanup lines
cy.get("[ui5-busy-indicator]").then(($el) => {
const element = $el[0] as any;

// Set active to trigger timeout creation
element.active = true;

// Manually trigger onBeforeRendering to create the timeout
element.onBeforeRendering();

// Verify timeout was created (should not be busy yet)
expect(element._isBusy).to.be.false;
expect(element._busyTimeoutId).to.exist;

// Now set active to false - this should trigger the cleanup code
element.active = false;
element.onBeforeRendering();

// Verify cleanup happened
expect(element._busyTimeoutId).to.be.undefined;
expect(element._isBusy).to.be.false;
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

merge it with the test "should clear timeout when component becomes inactive"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants