feat(ct): angular component testing #27783
Conversation
sand4rt
force-pushed
the
hello-angular-ct
branch
from
aa9b3fe
to
71599da
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
What's out plan here, is it ready to land? Is it based on Younes's work or is this something different? |
There was a problem hiding this comment.
Great job @sand4rt!
Here are a couple of things that would be nice to solve before merging.
Let me know if you want me to contribute to your branch directly with some PRs.
The main discussion here is about vite plugin replacement and configuration.
Hey @pavelfeldman! I just shared my feedback with @sand4rt.
It doesn't matter anymore. There was just a misunderstanding. |
|
Hey guys, thanks for the comments! I'm AFK for a couple of days. Will hopefully look at the PR by the end of next week as well |
There was a problem hiding this comment.
The added logo is the Angular.js one, here's the link to the correct Angular logo.
sand4rt
force-pushed
the
hello-angular-ct
branch
from
720980b
to
71599da
Compare
Hi @edbzn, thanks for the input! Would you like to make the change by creating a PR against this branch? See #27783 (comment) for more details edit: Angular just released their new logo: https://angular.dev/press-kit |
sand4rt
force-pushed
the
hello-angular-ct
branch
from
71599da
to
8ccb3bb
Compare
This comment has been minimized.
This comment has been minimized.
|
I just noticed that the tests themselves are using Angular 15. |
|
Hey @sand4rt, I just created a This way, we can update the todo list as this:
Here are the advantages of moving the Angular vite plugin setup to the
The drawbacks are:
Of course, this is just a temporary solution until the Angular team releases an official vite plugin which will at least warranty major version compatibility. |
|
Hey @sand4rt are you available in the upcoming days or weeks so we can finish this 😊 |
Hey @yjaaidi, #29544 needs to be resolved before i can update. I could resolve the merge conflicts later on if needed, so don't let that hold you back. For the people willing to contribute; beta testing would also help a lot to ensure everything operates as expected, your feedback is very welcome! |
|
Thanks @sand4rt! I think that we have everything now 😊:
Cf. sand4rt#5 |
sand4rt
force-pushed
the
hello-angular-ct
branch
from
9e88b09
to
31e8817
Compare
|
Great great great work! Thank you!! |
sand4rt
force-pushed
the
hello-angular-ct
branch
from
31e8817
to
3226aa5
Compare
|
Understood, then let's wait for the merge and then continue the discussion. Congrats again. |
|
@yjaaidi I am running into some issues while testing this library.
Other than that it's working great so far. Thanks so much for this. |
Hi @chronospatian, what is the value of ctViteConfig: {
plugins: [analog()], // or [swc.vite(swcAngularUnpluginOptions())]
resolve: {
/* @angular/material is using "style" as a Custom Conditional export to expose prebuilt styles etc... */
conditions: ['style'],
},
}, |
|
@rainerhahnekamp note that there are some known limitations on what can be used in providers. |
|
@yjaaidi I am using the same config as you have it here: https://github.com/sand4rt/playwright/blob/hello-angular-ct/tests/components/ct-angular/playwright.config.mts
|
|
|
|
@chronospatain you might want to check out #23662 for the esm/cjs issues |
|
The last thing I did to get hooks working was to make sure |
In which file did you put it initially? |
|
@yjaaidi I initially put it in the spec file, like beforeEach. |
|
Are there plans to add support for testing Directives and Pipes? eg. await mount(`<div [myDirective]="value | myPipe"></div>`, {
imports: [MyDirective, MyPipe],
props: {
value: "abc"
}
}) |
| "typescript": "~5.2.0", | ||
| "zone.js": "~0.14.0" | ||
| }, | ||
| "peerDependencies": { |
There was a problem hiding this comment.
This seems to be the first time we declare peerDependencies, and we don't really like them. Where do we use them? Can we avoid declaring them as peers? Should we hard-depend on them, similar to depending on vite-plugin-solid in @playwright/experimental-ct-solid?
There was a problem hiding this comment.
Hi Dmitry, they are used in registerSource.mjs in order to create the testbed.
We have to depend on the same Angular major version as the user's. If we hard-depend on Angular 18, we can cause problems to users who didn't migrate to Angular 18 yet. Even worse, the tests could pass (as they are using Angular 18) and break when the app is built (using Angular 17 for example).
| export type ComponentEvents = Record<string, Function>; | ||
|
|
||
| export interface MountOptions<HooksConfig extends JsonObject, Component> { | ||
| props?: Partial<Component> | Record<string, unknown>, // TODO: filter props and handle signals |
There was a problem hiding this comment.
Could you expand a bit on the TODO?
There was a problem hiding this comment.
The types are quite loose now while they should be stricter. @yjaaidi you already made something for this right?
There was a problem hiding this comment.
Yes, exactly!
We implemented this here https://github.com/jscutlery/devkit/blob/353aeb322fecad64ba8b3667613deca537adb743/packages/playwright-ct-angular/src/index.ts#L35
IMHO, it's better if we add this later in a distinct PR to reduce the surface of this PR.
| traverse(node, { | ||
| enter: p => { | ||
| // Treat calls to mount and all identifiers in arguments as component usages. | ||
| // e.g. mount(MyComponent, { imports: [OtherComponent], providers: [Token]}) |
There was a problem hiding this comment.
So far, we like the idea of not depending on the usage, but only on declaration. It seems like Angular has a nice practice of putting components into Button.component.ts files, so perhaps we can use this convention and treat .component as a component file similar to .vue and .svelte?
There was a problem hiding this comment.
Solely relying on the file extension might not be a good idea because of:
- Support for imports/providers in Angular is a must have (see this for more info)
- Native web components don't have a common file extension: (see this for more info)
- Vue components are not always defined in a
.vuefile (see this for more info)
There was a problem hiding this comment.
Hmm... Could you elaborate on the imports/providers bit? I followed the link, and it seems to import AngularComponent, presumably from the AngularComponent.component file?
I also think that vue and lit are a different story, so I'd rather not mix them up in this PR aiming to support angular. We can have a separate discussion with pros and cons about it in #30269.
There was a problem hiding this comment.
I don't think that we can assume any suffix or file naming convention for these additional reasons:
- some users use different suffixes/extensions to express the responsibility of the component (e.g.
Page&.page.ts) - some users might use Analog SFCs (https://analogjs.org/docs/experimental/sfc#analog-sfcs)
- the
Componentsuffix might not last (Angular team had already presented the possibility of using components in template without importtemplate: '<MyButton/>'(not jsx though). If this happens, suffixes will probably be dropped. - the
providersoption can receive classes that are not suffixed either and that should be forwarded to the browser (e.g.mount(MyCmp, {providers: [MyService]}))
While the following approach works relatively well, it is limited. Nowadays, most providers are provided using functions that return the provider configuration provideAnimations().
There are multiple options:
- keep the transformer as it is but users who want to use provider function will have to move the logic to hooks or story/test container components or create provider constants in other files: Cf. https://github.com/jscutlery/devkit/blob/353aeb322fecad64ba8b3667613deca537adb743/tests/playwright-ct-angular-demo/src/recipe-search.component.pw.ts#L34
- forward function calls used in
providers - introduce a "compiler function" like
forward(MyService)that allows users to explicitly choose what should be transformed and "forwarded" to the browser.
There was a problem hiding this comment.
Hmm... Could you elaborate on the imports/providers bit? I followed the link, and it seems to import AngularComponent, presumably from the AngularComponent.component file?
I meant to say that imports/providers (especially providers) will receive classes that don't necessarily have a suffix.
What concerns do you have regarding the current change?
| @@ -0,0 +1,45 @@ | |||
| # See http://help.github.com/ignore-files/ for more about ignoring files. | |||
|
|
|||
There was a problem hiding this comment.
This is an impressive gitignore file! Was it generated by angular cli for you?
There was a problem hiding this comment.
Yeah, tests/components/ct-angular was generated by angular cli
| for (const hook of window.__pw_hooks_after_mount || []) | ||
| await hook({ hooksConfig }); | ||
|
|
||
| __pwFixtureRegistry.set(rootElement.id, fixture); |
There was a problem hiding this comment.
Why mapping from id, and not from the rootElement itself?
There was a problem hiding this comment.
Nice catch! AFAIK, we could use rootElement. @sand4rt?
| } from '@angular/platform-browser-dynamic/testing'; | ||
|
|
||
| /** @type {WeakMap<import('@angular/core/testing').ComponentFixture, Record<string, import('rxjs').Subscription>>} */ | ||
| const __pwOutputSubscriptionRegistry = new WeakMap(); |
There was a problem hiding this comment.
Instead of a WeakMap, we usually use a symbol. However, in this case it seems like we can combine this map with __pwFixtureRegistry?
// rootElement -> fixture, subscriptions
Map<Element, {
fixture: import('@angular/core/testing').ComponentFixture,
subscriptions: Record<string, import('rxjs').Subscription>
}>
There was a problem hiding this comment.
💯 that would be a better option indeed.
| throw new Error('Only standalone components are supported'); | ||
|
|
||
| TestBed.configureTestingModule({ | ||
| imports: [component.type], |
There was a problem hiding this comment.
If we aim to support components as parameters (for slots?), do we have to list all component types in this imports list?
There was a problem hiding this comment.
Right now, the best option is to let the user create a parent story/test-container component:
// test-container.ts
@Component({
imports: [A, B],
template: `<a/><b/>`
})
class TestContainer {
}
// test
mount(TestContainer);Otherwise, a more convenient option is to allow the option of providing a template to the mount function.
mount(`<a/><b/>`, {imports: [A, B]});There was a problem hiding this comment.
This implementation would be more in line with the other frameworks:
https://github.com/sand4rt/playwright-ct-angular/blob/e8a74aa88b88fec329b01661cb0e0235f44d0f5a/ct-angular/tests/slots.spec.ts#L1-L47, but we thought it would be better to merge the library first before moving on to discussing the options
|
The test fixture appears to be anchored to the wrong element. This causes some getters, matchers and locators to fail or behave unexpectedly. import { Component, ElementRef} from '@angular/core';
@Component({
standalone: true,
template: `<div>Test</div>`,
})
export class AttributeComponent {
constructor(element: ElementRef) {
element.nativeElement.setAttribute('name', 'hello')
}
}import { AttributeComponent } from '@/components/attribute.component';
import { expect, test } from '@playwright/experimental-ct-angular';
import type { HooksConfig } from 'playwright';
test('has name attribute', async ({ mount }) => {
const component = await mount<HooksConfig>(AttributeComponent);
// works
await expect(component.page().locator('#root').getAttribute('name')).resolves.toBe('hello');
// doesn't work
await expect(component.getAttribute('name')).resolves.toBe('hello');
// works
await expect(component.page().locator('#root')).toHaveAttribute('name', 'hello');
// doesn't work
await expect(component).toHaveAttribute('name', 'hello');
}); |
| async function __pwRenderComponent(component) { | ||
| const componentMetadata = reflectComponentType(component.type); | ||
| if (!componentMetadata?.isStandalone) | ||
| throw new Error('Only standalone components are supported'); | ||
|
|
||
| TestBed.configureTestingModule({ | ||
| imports: [component.type], | ||
| }); | ||
|
|
||
| await TestBed.compileComponents(); | ||
|
|
||
| const fixture = TestBed.createComponent(component.type); | ||
| fixture.nativeElement.id = 'root'; | ||
|
|
||
| __pwUpdateProps(fixture, component.props); | ||
| __pwUpdateEvents(fixture, component.on); | ||
|
|
||
| fixture.autoDetectChanges(); | ||
|
|
||
| return fixture; | ||
| } |
There was a problem hiding this comment.
TestBed.createComponent discards the root element, which causes the locator to break. Suggested refactor below should resolve this issue:
/**
* @param {ObjectComponent & MountOptions} component
* @param {HTMLElement} rootElement
*/
async function __pwRenderComponent(component, rootElement) {
const componentMetadata = reflectComponentType(component.type);
if (!componentMetadata?.isStandalone)
throw new Error('Only standalone components are supported');
TestBed.configureTestingModule({
imports: [component.type],
});
await TestBed.compileComponents();
const fixture = TestBed.createComponent(component.type)
rootElement.replaceChildren(fixture.nativeElement)
document.body.replaceChildren(rootElement)
__pwUpdateProps(fixture, component.props);
__pwUpdateEvents(fixture, component.on);
fixture.autoDetectChanges();
return fixture;
}There was a problem hiding this comment.
Nice catch @chronospatian!
This is related to the behavior of Playwright's #root >> internal:control=component which will use #root if it has multiple children or the child if there is a single child.
We should indeed insert the element inside the root element provided by playwright.
The best way of handling this should be by implementing the TestComponentRenderer service:
class PlaywrightTestComponentRenderer extends TestComponentRenderer {
constructor(rootElement) {
super();
this._rootElement = rootElement;
}
insertRootElement(testRootElementId) {
const testRootElement = document.createElement('div');
testRootElement.id = testRootElementId;
this._children.push(testRootElement);
this._rootElement.appendChild(testRootElement);
}
removeAllRootElements() {
for (const child of this._children)
this._rootElement.removeChild(child);
}
}
...
TestBed.configureTestingModule({
imports: [component.type],
providers: [
{
provide: TestComponentRenderer,
useValue: new PlaywrightTestComponentRenderer(rootElement)
}
]
});
await TestBed.compileComponents();
const fixture = TestBed.createComponent(component.type);
...Instead of setting the root id on the fixture.
Test results for "tests 1"1 flaky27343 passed, 662 skipped Merge workflow run. |
Test results for "tests 1"2 failed 2 flaky27009 passed, 610 skipped Merge workflow run. |
|
I get errors when I try to import other values from a component file. // @/components.counter.component
import { Component, EventEmitter, Input, Output } from '@angular/core';
// adding this line
export const count = 10
@Component({
template: `{{count}}`
})
export class CounterComponent {
count = count
}
import { test, expect } from '@playwright/experimental-ct-angular';
import { count, CounterComponent } from '@/components/counter.component';
test('should mount', async ({ mount }) => {
const component = await mount(CounterComponent);
await expect(component).toHaveText(`${count}`)
});When I try to run this test I get the following error SyntaxError: Cannot use import statement outside a module
at ../src/components/counter.component.ts:1
> 1 | import { Component, EventEmitter, Input, Output } from '@angular/core';
| ^
2 |
3 | @Component({
4 | standalone: true,
at Object.<anonymous> (/Git/playwright/tests/components/ct-angular/src/components/counter.component.ts:1:1)
at Object.<anonymous> (/Git/playwright/tests/components/ct-angular/tests/update.spec.ts:2:1)
Error: No tests found.
Make sure that arguments are regular expressions matching test files.
You may need to escape symbols like "$" or "*" and quote the arguments.If I set "type: module" in package.json, then I get a different error. Lastly, if I change the test slightly: export const count = { value: 10 }I get yet another error: SyntaxError: Identifier 'CounterComponent' has already been declaredThis is very confusing! I guess this is a limitation of the playwright component test, so the solution here would be to extract the variable to another file that has no Angular imports or component usages, or to not import those values into the test to begin with and use hard coded values in the test. |
Hi @chronospatian, could you please provide a repro? |
|
@yjaaidi Here's the reproduction: https://github.com/chronospatian/playwright/blob/debug-component-usages/tests/components/ct-angular/tests/update.spec.ts I found a partial workaround. The test passes if I keep component usages and non-component usages in separate imports. I can even hold a reference to the component class itself if I import it a second time under an import alias. Neat! It would be nice if the typescript transform could discriminate between component and non-component usages in the same import statement. The test will always fail if the spec file tries to import any values from a file that contains references an Angular component with field decorators (such as Edit: Setting experimentalDecorators did not fix. I'm pretty sure you can't change Playwright's tsconfig except for baseUrl and paths. |
this is preparatory work for allowing zoneless testing
Test results for "tests 1"15 passed Merge workflow run. |
closes: #14153 and https://github.com/sand4rt/playwright-ct-angular
TODO
Enable vite-plugin-angular JIT mode: feat(vite-plugin-angular): add support for JIT mode analogjs/analog#374 (comment)Sourcemap is likely to be incorrect: a plugin (@analogjs/vite-plugin-angular)errors when transpiling: Sourcemap is likely to be incorrect: a plugin (@analogjs/vite-plugin-angular) analogjs/analog#410typeof plugin.defaultcheck: https://github.com/microsoft/playwright/pull/27783/files#diff-4592ac823d44ec894c518ba459cfab4bd544056a674739fda5fc145cdc596923R28@analogjs/vite-plugin-angularand related code and move it tocreate-playwright? feat(ct): angular component testing #27783 (comment)