Skip to content

Commit

Permalink
test(multiple): remove provideZoneChangeDetection for all menu tests (#…
Browse files Browse the repository at this point in the history
…29061)

This removes 'provideZoneChangeDetection' from menu tests. Change summary:

* Many tests set inputs directly. This isn't really how it happens in
  applications - inputs set from the template automatically mark
  components for check. When tests set inputs directly like this, they
  need to call `markForCheck` manually (or something similar).
* Similarly, test components need to be marked for check when setting
  values
* One test seems to be calling a public API (`CdkContextMenuTrigger.open`)
  that failed to call `markForCheck` so that was added to the implementation
* `fakeAsync` had an additional timer so I just added `flush` to make it
  work.

(cherry picked from commit 325533a)
  • Loading branch information
atscott authored and mmalerba committed May 21, 2024
1 parent 6c1982b commit d6146b9
Show file tree
Hide file tree
Showing 8 changed files with 45 additions and 47 deletions.
14 changes: 2 additions & 12 deletions src/cdk/menu/context-menu-trigger.spec.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,4 @@
import {
Component,
ViewChild,
ElementRef,
Type,
ViewChildren,
QueryList,
provideZoneChangeDetection,
} from '@angular/core';
import {Component, ViewChild, ElementRef, Type, ViewChildren, QueryList} from '@angular/core';
import {CdkMenuModule} from './menu-module';
import {TestBed, waitForAsync, ComponentFixture} from '@angular/core/testing';
import {CdkMenu} from './menu';
Expand All @@ -26,7 +18,6 @@ describe('CdkContextMenuTrigger', () => {
TestBed.configureTestingModule({
imports: [CdkMenuModule],
declarations: [SimpleContextMenu],
providers: [provideZoneChangeDetection()],
}).compileComponents();
}));

Expand Down Expand Up @@ -160,7 +151,6 @@ describe('CdkContextMenuTrigger', () => {
beforeEach(waitForAsync(() => {
TestBed.configureTestingModule({
imports: [CdkMenuModule],
providers: [provideZoneChangeDetection()],
declarations: [NestedContextMenu],
}).compileComponents();
}));
Expand Down Expand Up @@ -223,6 +213,7 @@ describe('CdkContextMenuTrigger', () => {
' is disabled',
() => {
fixture.componentInstance.copyMenuDisabled = true;
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();
openCopyContextMenu();

Expand Down Expand Up @@ -410,7 +401,6 @@ describe('CdkContextMenuTrigger', () => {
function createComponent<T>(componentClass: Type<T>) {
TestBed.configureTestingModule({
imports: [CdkMenuModule],
providers: [provideZoneChangeDetection()],
declarations: [componentClass],
}).compileComponents();

Expand Down
13 changes: 12 additions & 1 deletion src/cdk/menu/context-menu-trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,15 @@
* found in the LICENSE file at https://angular.io/license
*/

import {booleanAttribute, Directive, inject, Injectable, Input, OnDestroy} from '@angular/core';
import {
booleanAttribute,
ChangeDetectorRef,
Directive,
inject,
Injectable,
Input,
OnDestroy,
} from '@angular/core';
import {Directionality} from '@angular/cdk/bidi';
import {
FlexibleConnectedPositionStrategy,
Expand Down Expand Up @@ -83,6 +91,8 @@ export class CdkContextMenuTrigger extends CdkMenuTriggerBase implements OnDestr
/** The app's context menu tracking registry */
private readonly _contextMenuTracker = inject(ContextMenuTracker);

private readonly _changeDetectorRef = inject(ChangeDetectorRef);

/** Whether the context menu is disabled. */
@Input({alias: 'cdkContextMenuDisabled', transform: booleanAttribute}) disabled: boolean = false;

Expand All @@ -97,6 +107,7 @@ export class CdkContextMenuTrigger extends CdkMenuTriggerBase implements OnDestr
*/
open(coordinates: ContextMenuCoordinates) {
this._open(null, coordinates);
this._changeDetectorRef.markForCheck();
}

/** Close the currently opened context menu. */
Expand Down
4 changes: 2 additions & 2 deletions src/cdk/menu/menu-item-checkbox.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {Component, ElementRef, provideZoneChangeDetection} from '@angular/core';
import {Component, ElementRef} from '@angular/core';
import {ComponentFixture, TestBed, waitForAsync} from '@angular/core/testing';
import {By} from '@angular/platform-browser';
import {CdkMenuModule} from './menu-module';
Expand All @@ -16,7 +16,6 @@ describe('MenuItemCheckbox', () => {
TestBed.configureTestingModule({
imports: [CdkMenuModule, SingleCheckboxButton],
providers: [
provideZoneChangeDetection(),
{provide: CDK_MENU, useClass: CdkMenu},
{provide: MENU_STACK, useClass: MenuStack},
// View engine can't figure out the ElementRef to inject so we need to provide a fake
Expand All @@ -43,6 +42,7 @@ describe('MenuItemCheckbox', () => {
expect(checkboxElement.getAttribute('aria-disabled')).toBeNull();

checkbox.disabled = true;
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

expect(checkboxElement.getAttribute('aria-disabled')).toBe('true');
Expand Down
4 changes: 2 additions & 2 deletions src/cdk/menu/menu-item-radio.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {Component, ElementRef, provideZoneChangeDetection} from '@angular/core';
import {Component, ElementRef} from '@angular/core';
import {ComponentFixture, TestBed, waitForAsync} from '@angular/core/testing';
import {By} from '@angular/platform-browser';
import {UniqueSelectionDispatcher} from '@angular/cdk/collections';
Expand All @@ -19,7 +19,6 @@ describe('MenuItemRadio', () => {
TestBed.configureTestingModule({
imports: [CdkMenuModule, SimpleRadioButton],
providers: [
provideZoneChangeDetection(),
{provide: UniqueSelectionDispatcher, useValue: selectionDispatcher},
{provide: CDK_MENU, useClass: CdkMenu},
{provide: MENU_STACK, useClass: MenuStack},
Expand Down Expand Up @@ -47,6 +46,7 @@ describe('MenuItemRadio', () => {
expect(radioElement.getAttribute('aria-disabled')).toBeNull();

radioButton.disabled = true;
fixture.componentRef.changeDetectorRef.markForCheck();
fixture.detectChanges();

expect(radioElement.getAttribute('aria-disabled')).toBe('true');
Expand Down
9 changes: 6 additions & 3 deletions src/cdk/menu/menu-item.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {Component, Type, ElementRef, provideZoneChangeDetection} from '@angular/core';
import {Component, Type, ElementRef} from '@angular/core';
import {ComponentFixture, TestBed, waitForAsync} from '@angular/core/testing';
import {dispatchKeyboardEvent} from '@angular/cdk/testing/private';
import {By} from '@angular/platform-browser';
Expand All @@ -20,7 +20,6 @@ describe('MenuItem', () => {
imports: [CdkMenuModule],
declarations: [SingleMenuItem],
providers: [
provideZoneChangeDetection(),
{provide: CDK_MENU, useClass: CdkMenu},
{provide: MENU_STACK, useClass: MenuStack},
// View engine can't figure out the ElementRef to inject so we need to provide a fake
Expand Down Expand Up @@ -49,11 +48,13 @@ describe('MenuItem', () => {
expect(nativeButton.hasAttribute('aria-disabled')).toBeFalse();

menuItem.disabled = true;
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

expect(nativeButton.getAttribute('aria-disabled')).toBe('true');

menuItem.disabled = false;
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

expect(nativeButton.hasAttribute('aria-disabled')).toBeFalse();
Expand Down Expand Up @@ -83,7 +84,6 @@ describe('MenuItem', () => {
TestBed.configureTestingModule({
imports: [CdkMenuModule, MatIcon],
providers: [
provideZoneChangeDetection(),
{provide: CDK_MENU, useClass: CdkMenu},
{provide: MENU_STACK, useClass: MenuStack},
// View engine can't figure out the ElementRef to inject so we need to provide a fake
Expand All @@ -108,6 +108,7 @@ describe('MenuItem', () => {
const fixture = createComponent(MenuItemWithIcon);
expect(menuItem.getLabel()).toEqual('unicorn Click me!');
fixture.componentInstance.typeahead = 'Click me!';
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();
expect(menuItem.getLabel()).toEqual('Click me!');
});
Expand All @@ -119,6 +120,7 @@ describe('MenuItem', () => {
const fixture = createComponent(MenuItemWithIconClass);
expect(menuItem.getLabel()).toEqual('unicorn Click me!');
fixture.componentInstance.typeahead = 'Click me!';
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();
expect(menuItem.getLabel()).toEqual('Click me!');
},
Expand All @@ -136,6 +138,7 @@ describe('MenuItem', () => {
const fixture = createComponent(MenuItemWithMultipleNestings);
expect(menuItem.getLabel()).toEqual('unicorn Click menume!');
fixture.componentInstance.typeahead = 'Click me!';
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();
expect(menuItem.getLabel()).toEqual('Click me!');
},
Expand Down
15 changes: 5 additions & 10 deletions src/cdk/menu/menu-trigger.spec.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,4 @@
import {
Component,
ViewChildren,
QueryList,
ElementRef,
ViewChild,
Type,
provideZoneChangeDetection,
} from '@angular/core';
import {Component, ViewChildren, QueryList, ElementRef, ViewChild, Type} from '@angular/core';
import {ComponentFixture, TestBed, fakeAsync, tick, waitForAsync} from '@angular/core/testing';
import {By} from '@angular/platform-browser';
import {dispatchKeyboardEvent} from '../../cdk/testing/private';
Expand All @@ -27,7 +19,6 @@ describe('MenuTrigger', () => {
TestBed.configureTestingModule({
imports: [CdkMenuModule],
declarations: [TriggerForEmptyMenu],
providers: [provideZoneChangeDetection()],
}).compileComponents();
}));

Expand All @@ -51,6 +42,7 @@ describe('MenuTrigger', () => {
expect(menuItemElement.getAttribute('aria-disabled')).toBeNull();

menuItem.disabled = true;
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

expect(menuItemElement.getAttribute('aria-disabled')).toBe('true');
Expand All @@ -60,6 +52,7 @@ describe('MenuTrigger', () => {
expect(menuItemElement.getAttribute('aria-haspopup')).toEqual('menu');

fixture.componentInstance.trigger.menuTemplateRef = null;
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

expect(menuItemElement.hasAttribute('aria-haspopup')).toBe(false);
Expand All @@ -69,6 +62,7 @@ describe('MenuTrigger', () => {
expect(menuItem.hasMenu).toBeTrue();

fixture.componentInstance.trigger.menuTemplateRef = null;
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

expect(menuItem.hasMenu).toBeFalse();
Expand All @@ -83,6 +77,7 @@ describe('MenuTrigger', () => {
expect(menuItemElement.getAttribute('aria-expanded')).toBe('false');

fixture.componentInstance.trigger.menuTemplateRef = null;
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

expect(menuItemElement.hasAttribute('aria-expanded')).toBeFalse();
Expand Down
11 changes: 2 additions & 9 deletions src/cdk/menu/pointer-focus-tracker.spec.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,4 @@
import {
Component,
QueryList,
ElementRef,
ViewChildren,
AfterViewInit,
provideZoneChangeDetection,
} from '@angular/core';
import {Component, QueryList, ElementRef, ViewChildren, AfterViewInit} from '@angular/core';
import {waitForAsync, ComponentFixture, TestBed} from '@angular/core/testing';
import {createMouseEvent, dispatchEvent} from '../../cdk/testing/private';
import {Observable} from 'rxjs';
Expand All @@ -26,7 +19,6 @@ describe('FocusMouseManger', () => {

beforeEach(waitForAsync(() => {
TestBed.configureTestingModule({
providers: [provideZoneChangeDetection()],
imports: [MultiElementWithConditionalComponent, MockWrapper],
}).compileComponents();
}));
Expand Down Expand Up @@ -68,6 +60,7 @@ describe('FocusMouseManger', () => {

expect(mockElements.length).toBe(2);
fixture.componentInstance.showThird = true;
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();
getComponentsForTesting();

Expand Down
22 changes: 14 additions & 8 deletions src/material/menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import {
Type,
ViewChild,
ViewChildren,
provideZoneChangeDetection,
} from '@angular/core';
import {ComponentFixture, fakeAsync, flush, TestBed, tick} from '@angular/core/testing';
import {MatRipple} from '@angular/material/core';
Expand Down Expand Up @@ -64,7 +63,7 @@ describe('MDC-based MatMenu', () => {
declarations: any[] = [],
): ComponentFixture<T> {
TestBed.configureTestingModule({
providers: [provideZoneChangeDetection(), ...providers],
providers,
imports: [MatMenuModule, NoopAnimationsModule],
declarations: [component, ...declarations],
}).compileComponents();
Expand Down Expand Up @@ -96,6 +95,7 @@ describe('MDC-based MatMenu', () => {
expect(triggerElement.getAttribute('aria-haspopup')).toBe('menu');

fixture.componentInstance.trigger.menu = null;
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

expect(triggerElement.hasAttribute('aria-haspopup')).toBe(false);
Expand Down Expand Up @@ -371,6 +371,7 @@ describe('MDC-based MatMenu', () => {

// Add 50 items to make the menu scrollable
fixture.componentInstance.extraItems = new Array(50).fill('Hello there');
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

const triggerEl = fixture.componentInstance.triggerEl.nativeElement;
Expand Down Expand Up @@ -650,6 +651,7 @@ describe('MDC-based MatMenu', () => {
instance.ariaLabel = 'Custom aria-label';
instance.ariaLabelledby = 'custom-labelled-by';
instance.ariaDescribedby = 'custom-described-by';
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

expect(menuPanel.getAttribute('aria-label')).toBe('Custom aria-label');
Expand All @@ -658,6 +660,7 @@ describe('MDC-based MatMenu', () => {

// Change these to empty strings to make sure that we don't preserve empty attributes.
instance.ariaLabel = instance.ariaLabelledby = instance.ariaDescribedby = '';
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

expect(menuPanel.hasAttribute('aria-label')).toBe(false);
Expand Down Expand Up @@ -904,11 +907,11 @@ describe('MDC-based MatMenu', () => {
}));

it('should throw if assigning a menu that contains the trigger', fakeAsync(() => {
expect(() => {
const fixture = createComponent(InvalidRecursiveMenu, [], [FakeIcon]);
fixture.detectChanges();
tick(500);
}).toThrowError(/menu cannot contain its own trigger/);
const errorSpy = spyOn(console, 'error');
const fixture = createComponent(InvalidRecursiveMenu, [], [FakeIcon]);
fixture.detectChanges();
tick(500);
expect(errorSpy.calls.first().args[1]).toMatch(/menu cannot contain its own trigger/);
}));

it('should be able to swap out a menu after the first time it is opened', fakeAsync(() => {
Expand Down Expand Up @@ -1222,6 +1225,7 @@ describe('MDC-based MatMenu', () => {
expect(Math.floor(panelRect.bottom)).toBeLessThan(viewportHeight);

fixture.componentInstance.extraItems = new Array(50).fill('Hello there');
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();
panelRect = panel.getBoundingClientRect();
expect(Math.floor(panelRect.bottom)).toBe(viewportHeight);
Expand Down Expand Up @@ -1408,6 +1412,7 @@ describe('MDC-based MatMenu', () => {
trigger.style.top = '200px';

fixture.componentInstance.yPosition = 'above';
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

fixture.componentInstance.trigger.openMenu();
Expand All @@ -1425,6 +1430,7 @@ describe('MDC-based MatMenu', () => {
tick(500);

fixture.componentInstance.yPosition = 'below';
fixture.changeDetectorRef.markForCheck();
fixture.detectChanges();

fixture.componentInstance.trigger.openMenu();
Expand Down Expand Up @@ -2499,6 +2505,7 @@ describe('MDC-based MatMenu', () => {
fixture.detectChanges();
tick(500);
fixture.detectChanges();
flush();

expect(lazyTrigger.classList)
.withContext('Expected the trigger to be highlighted')
Expand Down Expand Up @@ -2711,7 +2718,6 @@ describe('MatMenu default overrides', () => {
TestBed.configureTestingModule({
imports: [MatMenuModule, NoopAnimationsModule],
providers: [
provideZoneChangeDetection(),
{
provide: MAT_MENU_DEFAULT_OPTIONS,
useValue: {overlapTrigger: true, xPosition: 'before', yPosition: 'above'},
Expand Down

0 comments on commit d6146b9

Please sign in to comment.