Skip to content

Commit

Permalink
fix(cdk/testing): incorrectly handling ancestor of compound selector (#…
Browse files Browse the repository at this point in the history
…22476)

Currently the component harness handles the `ancestor` option by prepending it to the `hostSelector` of the harness. This breaks down if the `hostSelector` is a compound selector, because we end up with something like `.parent .a, .b`, rather than `.parent .a, .parent .b`.

These changes resolve the issue and add a bit of extra logic to account for the case where the selector might include commas inside quotes which will break us, because we split compound selectors on the comma. The logic is loosely based on my changes from angular/angular#38716.

Fixes #22475.

(cherry picked from commit 032feef)
  • Loading branch information
crisbeto authored and mmalerba committed May 7, 2021
1 parent 97c449a commit ea84e7d
Show file tree
Hide file tree
Showing 6 changed files with 123 additions and 3 deletions.
53 changes: 50 additions & 3 deletions src/cdk/testing/component-harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -520,9 +520,25 @@ export class HarnessPredicate<T extends ComponentHarness> {

/** Gets the selector used to find candidate elements. */
getSelector() {
return this._ancestor.split(',')
.map(part => `${part.trim()} ${this.harnessType.hostSelector}`.trim())
.join(',');
// We don't have to go through the extra trouble if there are no ancestors.
if (!this._ancestor) {
return (this.harnessType.hostSelector || '').trim();
}

const [ancestors, ancestorPlaceholders] = _splitAndEscapeSelector(this._ancestor);
const [selectors, selectorPlaceholders] =
_splitAndEscapeSelector(this.harnessType.hostSelector || '');
const result: string[] = [];

// We have to add the ancestor to each part of the host compound selector, otherwise we can get
// incorrect results. E.g. `.ancestor .a, .ancestor .b` vs `.ancestor .a, .b`.
ancestors.forEach(escapedAncestor => {
const ancestor = _restoreSelector(escapedAncestor, ancestorPlaceholders);
return selectors.forEach(escapedSelector =>
result.push(`${ancestor} ${_restoreSelector(escapedSelector, selectorPlaceholders)}`));
});

return result.join(', ');
}

/** Adds base options common to all harness types. */
Expand Down Expand Up @@ -560,3 +576,34 @@ function _valueAsString(value: unknown) {
return '{...}';
}
}

/**
* Splits up a compound selector into its parts and escapes any quoted content. The quoted content
* has to be escaped, because it can contain commas which will throw throw us off when trying to
* split it.
* @param selector Selector to be split.
* @returns The escaped string where any quoted content is replaced with a placeholder. E.g.
* `[foo="bar"]` turns into `[foo=__cdkPlaceholder-0__]`. Use `_restoreSelector` to restore
* the placeholders.
*/
function _splitAndEscapeSelector(selector: string): [parts: string[], placeholders: string[]] {
const placeholders: string[] = [];

// Note that the regex doesn't account for nested quotes so something like `"ab'cd'e"` will be
// considered as two blocks. It's a bit of an edge case, but if we find that it's a problem,
// we can make it a bit smarter using a loop. Use this for now since it's more readable and
// compact. More complete implementation:
// https://github.com/angular/angular/blob/bd34bc9e89f18a/packages/compiler/src/shadow_css.ts#L655
const result = selector.replace(/(["'][^["']*["'])/g, (_, keep) => {
const replaceBy = `__cdkPlaceholder-${placeholders.length}__`;
placeholders.push(keep);
return replaceBy;
});

return [result.split(',').map(part => part.trim()), placeholders];
}

/** Restores a selector whose content was escaped in `_splitAndEscapeSelector`. */
function _restoreSelector(selector: string, placeholders: string[]): string {
return selector.replace(/__cdkPlaceholder-(\d+)__/g, (_, index) => placeholders[+index]);
}
16 changes: 16 additions & 0 deletions src/cdk/testing/tests/cross-environment.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,22 @@ export function crossEnvironmentSpecs(
const subcomps = await harness.directAncestorSelectorSubcomponent();
expect(subcomps.length).toBe(2);
});

it('should handle a compound selector with an ancestor', async () => {
const elements = await harness.compoundSelectorWithAncestor();

expect(await parallel(() => elements.map(element => element.getText()))).toEqual([
'Div inside parent',
'Span inside parent'
]);
});

it('should handle a selector with comma inside attribute with an ancestor', async () => {
const element = await harness.quotedContentSelectorWithAncestor();

expect(element).toBeTruthy();
expect(await element.getText()).toBe('Has comma inside attribute');
});
});

describe('HarnessPredicate', () => {
Expand Down
21 changes: 21 additions & 0 deletions src/cdk/testing/tests/harnesses/compound-selector-harness.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {ComponentHarness, HarnessPredicate} from '../../component-harness';

export class CompoundSelectorHarness extends ComponentHarness {
static readonly hostSelector = '.some-div, .some-span';

static with(options = {}) {
return new HarnessPredicate(CompoundSelectorHarness, options);
}

async getText(): Promise<string> {
return (await this.host()).text();
}
}
6 changes: 6 additions & 0 deletions src/cdk/testing/tests/harnesses/main-component-harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

import {ComponentHarness} from '../../component-harness';
import {TestElement, TestKey} from '../../test-element';
import {CompoundSelectorHarness} from './compound-selector-harness';
import {QuotedCommaSelectorHarness} from './quoted-comma-selector-harness';
import {SubComponentHarness, SubComponentSpecialHarness} from './sub-component-harness';

export class WrongComponentHarness extends ComponentHarness {
Expand Down Expand Up @@ -81,6 +83,10 @@ export class MainComponentHarness extends ComponentHarness {
this.locatorForAll(SubComponentHarness.with({ancestor: '.other, .subcomponents'}));
readonly directAncestorSelectorSubcomponent =
this.locatorForAll(SubComponentHarness.with({ancestor: '.other >'}));
readonly compoundSelectorWithAncestor =
this.locatorForAll(CompoundSelectorHarness.with({ancestor: '.parent'}));
readonly quotedContentSelectorWithAncestor =
this.locatorFor(QuotedCommaSelectorHarness.with({ancestor: '.quoted-comma-parent'}));

readonly subcomponentHarnessesAndElements =
this.locatorForAll('#counter', SubComponentHarness);
Expand Down
21 changes: 21 additions & 0 deletions src/cdk/testing/tests/harnesses/quoted-comma-selector-harness.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {ComponentHarness, HarnessPredicate} from '../../component-harness';

export class QuotedCommaSelectorHarness extends ComponentHarness {
static readonly hostSelector = 'div[has-comma="a,b"]';

static with(options = {}) {
return new HarnessPredicate(QuotedCommaSelectorHarness, options);
}

async getText(): Promise<string> {
return (await this.host()).text();
}
}
9 changes: 9 additions & 0 deletions src/cdk/testing/tests/test-main-component.html
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,15 @@ <h1 style="height: 100px; width: 200px;">Main Component</h1>
<test-sub title="other 1"></test-sub>
<test-sub title="other 2"></test-sub>
</div>
<div class="parent">
<div class="some-div">Div inside parent</div>
<div class="some-span">Span inside parent</div>
</div>
<div class="some-div">Div outside parent</div>
<div class="some-span">Span outside parent</div>
<div class="quoted-comma-parent">
<div has-comma="a,b">Has comma inside attribute</div>
</div>
<div class="task-state-tests">
<button (click)="runTaskOutsideZone()" id="task-state-test-trigger">
Run task outside zone
Expand Down

0 comments on commit ea84e7d

Please sign in to comment.