-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
feat(ct): angular component testing #27783
base: main
Are you sure you want to change the base?
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The added logo is the Angular.js one, here's the link to the correct Angular logo.
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 |
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 |
9e88b09
to
31e8817
Compare
Great great great work! Thank you!! |
31e8817
to
3226aa5
Compare
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you expand a bit on the TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.vue
file (see this for more info)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Component
suffix 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
providers
option 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an impressive gitignore file! Was it generated by angular cli for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why mapping from id, and not from the rootElement
itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the locator fix these tests should be updated.
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 declared This 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. |
@dgozman we are currently waiting for a response to resolve the following comments:
We also have to take care of the following:
|
Hi @zargham-leanix, i think the team is busy. meanwhile, you can use our community support |
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.default
check: https://github.com/microsoft/playwright/pull/27783/files#diff-4592ac823d44ec894c518ba459cfab4bd544056a674739fda5fc145cdc596923R28@analogjs/vite-plugin-angular
and related code and move it tocreate-playwright
? feat(ct): angular component testing #27783 (comment)