Skip to content
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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

sand4rt
Copy link
Collaborator

@sand4rt sand4rt commented Oct 24, 2023

closes: #14153 and https://github.com/sand4rt/playwright-ct-angular

TODO

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@pavelfeldman
Copy link
Member

What's out plan here, is it ready to land? Is it based on Younes's work or is this something different?

Copy link

@yjaaidi yjaaidi left a 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.

packages/playwright-ct-angular/registerSource.mjs Outdated Show resolved Hide resolved
packages/playwright-ct-angular/registerSource.mjs Outdated Show resolved Hide resolved
packages/playwright-ct-angular/index.js Outdated Show resolved Hide resolved
packages/playwright-ct-angular/package.json Outdated Show resolved Hide resolved
@yjaaidi
Copy link

yjaaidi commented Oct 25, 2023

What's out plan here, is it ready to land?

Hey @pavelfeldman! I just shared my feedback with @sand4rt.
Once we agree on that and fix what can be fixed, the PR should be ready.

Is it based on Younes's work or is this something different?

It doesn't matter anymore. There was just a misunderstanding.
We were just a bit disappointed for not merging efforts but now we are working together to provide what the community is waiting for and that's what matters.

@sand4rt
Copy link
Collaborator Author

sand4rt commented Oct 27, 2023

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

Copy link

@edbzn edbzn left a 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.

packages/playwright-ct-angular/index.js Outdated Show resolved Hide resolved
@sand4rt
Copy link
Collaborator Author

sand4rt commented Nov 5, 2023

The added logo is the Angular.js one, here's the link to the correct Angular logo.

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

This comment has been minimized.

@yjaaidi
Copy link

yjaaidi commented Nov 13, 2023

I just noticed that the tests themselves are using Angular 15.
Should we create a different folder for Angular 16 & Angular 17 and that would require lots of duplication?
Or should we do something at the test-all.spec.js level but that would be a bit too magical?
Or should the "experimental" CT only handle the last version of the framework?
cc. @mxschmitt @pavelfeldman @sand4rt

@yjaaidi
Copy link

yjaaidi commented Nov 21, 2023

Hey @sand4rt, I just created a create-playwright PR microsoft/create-playwright#106 to move the Angular vite plugin config to "user-land".

This way, we can update the todo list as this:

  • Do not handle routing and let users use router testing harness
  • Update tests to use Angular 17
  • Update the documentation
  • Remove @analogjs/vite-plugin-angular dependency & setup.
  • Update https://github.com/microsoft/create-playwright to generate playwright config with @analogjs/vite-plugin-angular and add it to project dependencies.

Here are the advantages of moving the Angular vite plugin setup to the create-playwright generator:

  • @analogjs/vite-plugin-angular has to catch up with Angular internals to make it work for future versions. Currently, the Angular internals it relies on can break at any Angular minor or patch version. Meaning that @playwright/experimental-ct-angular will have to catch up to by updating the dependency which also means a new Playwright release.
  • Users can easily choose the @analogjs/vite-plugin-angular version of their choice.
  • Users can easily choose the @analogjs/vite-plugin-angular configuration of their choice.
  • Users can easily choose another plugin.
  • This doesn't add any dependency to this workspace.

The drawbacks are:

  • It doesn't work out of the box if users don't use the generator or if they use the package manually without reading the docs.
  • Users will have to manually keep @analogjs/vite-plugin-angular up-to-date.

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.

@yjaaidi
Copy link

yjaaidi commented Feb 15, 2024

Hey @sand4rt are you available in the upcoming days or weeks so we can finish this 😊
Maybe if you could just update your branch I could send you a PR.
Thanks!

@flobacher
Copy link

Great news @yjaaidi and @sand4rt! Thank you so much for your effort time and persistence!!!

@sand4rt
Copy link
Collaborator Author

sand4rt commented Feb 22, 2024

Maybe if you could just update your branch I could send you a PR.

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!

@yjaaidi
Copy link

yjaaidi commented Feb 23, 2024

Thanks @sand4rt!
@edbzn and I had to update and fix #29544 before moving forward.

I think that we have everything now 😊:

  • ✨ it works with thew new transformation
  • ✨ template rendering (thanks to the new transformation system, amazing job @pavelfeldman)
  • ✨ signal inputs support
  • ✨ moved out analog vite plugin from the package
  • ✨ Angular 17 support
  • ✨ and passing providers

Cf. sand4rt#5

@maartentibau
Copy link

Great great great work! Thank you!!
At this moment this will only support standalone components right?

@zargham-leanix
Copy link

Great work @sand4rt @yjaaidi
When this PR is expected to be merged ?

This comment has been minimized.

@sand4rt
Copy link
Collaborator Author

sand4rt commented May 10, 2024

Hey @pavelfeldman, the PR is finally set for review by the Playwright team! We have deliberately kept the adapter minimal for now and postponed the documentation until more people have tested it and shared their feedback. We would like to gradually make improvements here and there after the merge.

@rainerhahnekamp
Copy link
Contributor

Congratulations to all of you. That's a milestone in Angular testing history!

Just a small remark: From what I've seen, there is no possibility to provide services, right?

If that's the case, it would be great to add this as the next feature once this branch is merged.

As you know, the vast majority of Angular Components usually depend on Services. If we could mock these services, the number of people who could test on a large scale would increase "exponentially."

Either way, congratulations and a thousand thanks again. I'm looking forward to it

@sand4rt
Copy link
Collaborator Author

sand4rt commented May 11, 2024

Congratulations to all of you. That's a milestone in Angular testing history!

Just a small remark: From what I've seen, there is no possibility to provide services, right?

If that's the case, it would be great to add this as the next feature once this branch is merged.

As you know, the vast majority of Angular Components usually depend on Services. If we could mock these services, the number of people who could test on a large scale would increase "exponentially."

Either way, congratulations and a thousand thanks again. I'm looking forward to it

Providers/imports are still open for discussion indeed. We've decided to merge the library first before continuing the discussion: sand4rt#5 (comment)

I think it could either be supported like the other frameworks through the hooksConfig:

// playwright/index.ts
beforeMount(async ({ TestBed, hooksConfig }) => {
  TestBed.configureTestingModule({
    imports: hooksConfig?.imports,
    providers: hooksConfig?.providers
  });
});
test('hooks config', async ({ mount }) => {
  const component = await mount(AngularComponent, {
    hooksConfig: {
      imports: [Component],
      providers: [Service],
    }
  });
  await expect(component).toContainText('boop');
});

or with dedicated APIs:

test('imports and providers', async ({ mount }) => {
  const component = await mount(AngularComponent, {
    imports: [Component],
    providers: [Service],
  });
  await expect(component).toContainText('boop');
});

@rainerhahnekamp
Copy link
Contributor

Understood, then let's wait for the merge and then continue the discussion. Congrats again.

@maartentibau
Copy link

Amazing work @sand4rt @yjaaidi ... this really is a milestone.
Can't thank you guys enough for this effort! 👏 👏 👏

@chronospatian
Copy link

chronospatian commented May 13, 2024

@yjaaidi I am running into some issues while testing this library.
sand4rt/playwright-ct-angular#50 Resolved
sand4rt/playwright-ct-angular#51 Resolved
sand4rt/playwright-ct-angular#52 Resolved

I have pulled and linked the PR branch and still face these errors. I also cannot import from @angular/material without adding "type": "module" to my package.json, or changing all my files to .mts.

Other than that it's working great so far. Thanks so much for this.

@yjaaidi
Copy link

yjaaidi commented May 13, 2024

@yjaaidi I am running into some issues while testing this library. sand4rt/playwright-ct-angular#50 sand4rt/playwright-ct-angular#51 Resolved sand4rt/playwright-ct-angular#52 Resolved

I have pulled and linked the PR branch and still face these errors. I also cannot import from @angular/material without adding "type": "module" to my package.json, or changing all my files to .mts.

Other than that it's working great so far. Thanks so much for this.

Hi @chronospatian, what is the value of ctViteConfig in the playwright config file?
Did you try this:

    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'],
      },
    },

@yjaaidi
Copy link

yjaaidi commented May 13, 2024

@rainerhahnekamp note that there are some known limitations on what can be used in providers.
We've documented this here https://github.com/jscutlery/devkit/tree/main/packages/playwright-ct-angular#%EF%B8%8F-known-limitations but we will add this to playwright docs once this is merged.

@chronospatian
Copy link

chronospatian commented May 13, 2024

@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

Except I also added globalThis.window = globalThis to work around window being undefined when using the beforeMount hook.

@chronospatian
Copy link

chronospatian commented May 13, 2024

Actually it doesn't appear that the beforeMount hook is being called at all... Not sure what I'm doing wrong. I was putting beforeMount in the wrong file, apologies.

@sand4rt
Copy link
Collaborator Author

sand4rt commented May 13, 2024

@chronospatain you might want to check out #23662 for the esm/cjs issues

@chronospatian
Copy link

The last thing I did to get hooks working was to make sure playwright/index.ts was included in the tsconfig files referenced by the vite plugin.

@yjaaidi
Copy link

yjaaidi commented May 13, 2024

I was putting beforeMount in the wrong file, apologies.

In which file did you put it initially?
I'm curious about common mistakes/problems. 😉

@chronospatian
Copy link

@yjaaidi I initially put it in the spec file, like beforeEach.

@chronospatian
Copy link

chronospatian commented May 14, 2024

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": {
Copy link
Contributor

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?

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
Copy link
Contributor

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?

Copy link
Collaborator Author

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?


/** @typedef {import('../playwright-ct-core/types/component').ObjectComponent} ObjectComponent */

import 'zone.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this one?

traverse(node, {
enter: p => {
// Treat calls to mount and all identifiers in arguments as component usages.
// e.g. mount(MyComponent, { imports: [OtherComponent], providers: [Token]})
Copy link
Contributor

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?

Copy link
Collaborator Author

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:

Copy link
Contributor

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.

@@ -0,0 +1,45 @@
# See http://help.github.com/ignore-files/ for more about ignoring files.

Copy link
Contributor

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?

Copy link
Collaborator Author

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

tests/components/ct-angular/src/assets/logo.svg Outdated Show resolved Hide resolved
for (const hook of window.__pw_hooks_after_mount || [])
await hook({ hooksConfig });

__pwFixtureRegistry.set(rootElement.id, fixture);
Copy link
Contributor

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?

} from '@angular/platform-browser-dynamic/testing';

/** @type {WeakMap<import('@angular/core/testing').ComponentFixture, Record<string, import('rxjs').Subscription>>} */
const __pwOutputSubscriptionRegistry = new WeakMap();
Copy link
Contributor

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>
}>

throw new Error('Only standalone components are supported');

TestBed.configureTestingModule({
imports: [component.type],
Copy link
Contributor

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?

@chronospatian
Copy link

chronospatian commented May 15, 2024

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');
});

Comment on lines +44 to +64
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;
}
Copy link

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;
}

Copy link

@chronospatian chronospatian left a 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.

@chronospatian
Copy link

@yjaaidi I've created a PR with fixes for the above and support for template strings sand4rt#6

Copy link
Contributor

Test results for "tests 1"

1 flaky ⚠️ [firefox-page] › page/frame-goto.spec.ts:46:3 › should continue after client redirect

27343 passed, 662 skipped
✔️✔️✔️

Merge workflow run.

Copy link
Contributor

Test results for "tests 1"

2 failed
❌ [playwright-test] › playwright.ct-react.spec.ts:253:5 › should pass "key" attribute from JSX in variable
❌ [installation tests] › playwright-electron-should-work.spec.ts:20:5 › electron should work

2 flaky ⚠️ [chromium-library] › library/capabilities.spec.ts:141:3 › should not crash on showDirectoryPicker
⚠️ [playwright-test] › ui-mode-test-screencast.spec.ts:21:5 › should show screenshots

27009 passed, 610 skipped
✔️✔️✔️

Merge workflow run.

@chronospatian
Copy link

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.

Error: Standard Angular field decorators are not supported in JIT mode.

    at PropDecorator (/Git/playwright/tests/packages/core/src/util/decorators.ts:161:17)
    at applyDec (/Git/playwright/tests/components/ct-angular/src/components/counter.component.ts:3:1813)
    at/Git/playwright/tests/components/ct-angular/src/components/counter.component.ts:3:3514
    at _applyDecs (/Git/playwright/tests/components/ct-angular/src/components/counter.component.ts:3:3685)
    at /Git/playwright/tests/components/ct-angular/src/components/counter.component.ts:23:2

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.

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.

[Feature] Support for Component Tests in Angular apps