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: add lit package for component tests #24250

Closed

Conversation

simonireilly
Copy link

@simonireilly simonireilly commented Oct 13, 2022

User facing changelog

Lit Component Testing Support

Additional details

Steps to test

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 13, 2022

Thanks for taking the time to open a PR!

@CLAassistant
Copy link

CLAassistant commented Oct 13, 2022

CLA assistant check
All committers have signed the CLA.

@simonireilly simonireilly changed the title Add lit package for component tests feat: add lit package for component tests Oct 13, 2022
@simonireilly simonireilly marked this pull request as draft October 13, 2022 21:04
* Mounts a Lit element inside the Cypress browser
*
* @param {LitElementConstructor<T>} Element Lit element being mounted
* @param {MountReturn<T extends SvelteComponent>} options options to customize the element being mounted
Copy link
Author

Choose a reason for hiding this comment

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

Todo, describe the template argument in jsDoc

Comment on lines 76 to 80
.wrap({ component: componentInstance }, { log: false })
.get('[data-cy-root]')
.children()
.first()
.shadow()
Copy link
Author

Choose a reason for hiding this comment

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

Does the bind make sense here, or can cause issue.

In theory it's not deterministic if mounting 2 components in 1 test (could return either depending on order).

Making the returned already bound to the shadow DOM is a better UX though.

Choose a reason for hiding this comment

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

To me, it feels reasonable to expect a user to only provide a single component to test. I don't know how cypress's root element is managed though. One way to make sure is to create like an empty div to use as a render target and return its first child to be tested.

That div that was created can then be cleaned up with a .remove() easily on cleanup, without affecting anything else that might be in the cypress root.

Also, probably would be better to return out the actual component element from the mount here and not dive into the shadow DOM. User may want to test against the element's attribute/property or component might've been authored to not use shadow DOM. I think it should be up to the user to intentionally dive into the shadow DOM.

Copy link

@augustjk augustjk left a comment

Choose a reason for hiding this comment

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

This is awesome to see!

I think this doesn't necessarily need to be lit specific since all web components can be tested in this way. The only thing lit specific in my comment is the awaiting on the updateComplete promise which doesn't have to be the only way. For instance: https://github.com/open-wc/open-wc/blob/eb28644e6789f6e349a66b7422717d543741be64/packages/testing-helpers/src/elementUpdated.js#L19

Open WC's test fixtures have been doing something like this that works nicely for component testing with Web Test Runner so I think it's worth taking some inspiration there.
https://open-wc.org/docs/testing/helpers/#test-fixtures

Comment on lines 76 to 80
.wrap({ component: componentInstance }, { log: false })
.get('[data-cy-root]')
.children()
.first()
.shadow()

Choose a reason for hiding this comment

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

To me, it feels reasonable to expect a user to only provide a single component to test. I don't know how cypress's root element is managed though. One way to make sure is to create like an empty div to use as a render target and return its first child to be tested.

That div that was created can then be cleaned up with a .remove() easily on cleanup, without affecting anything else that might be in the cypress root.

Also, probably would be better to return out the actual component element from the mount here and not dive into the shadow DOM. User may want to test against the element's attribute/property or component might've been authored to not use shadow DOM. I think it should be up to the user to intentionally dive into the shadow DOM.

}).snapshot('mounted').end()
}
})
.wrap({ component: componentInstance }, { log: false })

Choose a reason for hiding this comment

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

Is this necessary? componentInstance here is the return value of lit's render() which I don't think is actually usable here.


// by waiting, we are delaying test execution for the next tick of event loop
// and letting hooks and component lifecycle methods to execute mount
return cy.wait(0, { log: false }).then(() => {

Choose a reason for hiding this comment

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

Maybe there's a better cypress API for waiting on a specific promise. If we get the reference to the Lit element after render(), we can await on updateComplete promise to ensure its mounted. Awaiting on a promise resolved by requestAnimationFrame is another option if updateComplete is not found.

// and letting hooks and component lifecycle methods to execute mount
return cy.wait(0, { log: false }).then(() => {
if (options.log !== false) {
const mountMessage = `<${getComponentDisplayName(Component)} ... />`

Choose a reason for hiding this comment

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

Using the element's .tagName would give us the name without pulling from the Component class.

Comment on lines 53 to 54
Component: LitElementConstructor<T>,
Template: TemplateResult,

Choose a reason for hiding this comment

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

If the user registers their custom element with a side effect import, it should not be necessary to even pass in the component class into mount().

I think taking in a TemplateResult is not a bad start. It does mean user has to import html from lit in their test code. (Which I'd personally prefer anyway since it'll come with some checks with lit-plugin). But the API could also support the user providing just a string, then one can wrap it with lit's unsafeHTML directive and provide to html tag function.

This is the same take as open-wc's test fixture approach.

@simonireilly
Copy link
Author

simonireilly commented Oct 16, 2022

Thanks for your comments @augustjk - my current plan is to address most of the boiler plate code that Cypress has for generating test scenarios. Then focus on providing a good API implementation.

I agree with all your comments and will get round to addressing them.

Currently I'm working through dependency detection and encountered an error on require.resolve.

I've cross issued this to lit here: lit/lit#3368

Personally I think this issue points two ways:

  1. Packages consuming dependencies should not assume the package.json is in the exports.
  2. Packages which include the package.json in the distribution should define it within their exports.

I admint that the above trends towrds opinions and not a hard and fast rule.

Here is the code creating the issue:

const loc = require.resolve(path.join(dependency.package, 'package.json'), {
paths: [projectPath],
})

I haven't opened an issue in this repo as I may address it in this PR - but that might go against the contributing guidlines and increase the scope so I'll cc @ZachJW34 and see what makes sense at this point 👍

## Edit

Discussion can focus on this commit - e638787

It does use errors for for control flow which I don't like - alternatives is to extend the dependency object to define where the package.json is.

@ahnpnl
Copy link

ahnpnl commented Oct 20, 2022

looking forward to this!!!

Copy link
Author

@simonireilly simonireilly left a comment

Choose a reason for hiding this comment

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

THis has a passing test case for:

All of which are in Vite atm. The only outstanding todo for the code is to make the system tests pass for an equivalent webpack example.

@augustjk I think I have addressed all your comments in the mount file, but the API might have room for improvement: https://github.com/cypress-io/cypress/pull/24250/files#diff-959675e609b4b09e38a41a84b49dfcb59507e696dc6270505036da95e5dc0278

injectStylesBeforeElement(options, document, target)

// If give a string set internal contents unsafely
const element =
Copy link
Author

Choose a reason for hiding this comment

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

Is this what you had in mind @augustjk ?

Comment on lines +77 to +90
/**
* Pass properties to the component using the property assignment. This
* enables support for non string types. Not required when using
* lit-html.
*/
if (
properties &&
typeof properties === 'object' &&
Array.isArray(properties) === false
) {
Object.entries(properties).forEach(([key, value]) => {
el[(key as keyof typeof el)] = value
})
}
Copy link
Author

Choose a reason for hiding this comment

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

For custom elements defined without Lit we still want to make it easy for the user to override internals.

This technique enables injecting spies etc.

it("can pass emitters as spies", () => {
  cy.mount<"counter-wc">(
    `<counter-wc
      count=${42}
    ></counter-wc>`,
    { properties: { clicked: cy.spy().as("onClickedSpy") } }
  );

  cy.get("counter-wc").shadow().as("shadow");

  cy.get("@shadow").contains("h1", "Count is 42");
  cy.get("@shadow").find("button").click();
  cy.get("@onClickedSpy").should("have.been.calledWith", 42);
});

Comment on lines +67 to +69
.then(() => {
render(element, target)
})
Copy link
Author

Choose a reason for hiding this comment

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

Render the element into the DOM only if we find the root element which will old the mounted element.

Comment on lines +73 to +74
const name = element.prop('tagName').toLowerCase()
const el = document.getElementsByTagName<T>(name)[0]
Copy link
Author

Choose a reason for hiding this comment

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

This was done in order to get a reference to the element on the DOM after it has been rendered.

.end()
}

return cy.wrap({ component: el }, { log: false })
Copy link
Author

Choose a reason for hiding this comment

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

The return of the wrap will be a HTML element, the type will be narrowed by using the generic fetched from the HTMLElementTagNameMap.

Copy link

Choose a reason for hiding this comment

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

I just tested out locally, the new change makes this syntax no longer work

const element = cy.mount(html`<my-element></my-element>`)

    element.then(($el) => {
      console.warn($el.attr('data-foo')) // error `attr` doesn't exist in `$el`
    })

This syntax is normal Cypress syntax

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @ahnpnl.

The mount command returns a wrapped structure as:

{ component: element}

Your code will probably work if you destructive the return from the mount promise.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link

@ahnpnl ahnpnl Oct 23, 2022

Choose a reason for hiding this comment

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

Thanks! Indeed destructuring to use component would work. But should it be similar to cy.mount for Vue that we get immediately component object instead of having to destructuring it?

Another problem is when combining with beforeEach, I ran into error. Giving the codes

describe('<my-element />', () => {
  beforeEach(() => {
    cy.mount(html`<my-element></my-element>`);
  })
  
  it('renders', () => {
    console.log('renders);
  })
  
  it('not renders', () => {
        console.log('not renders');
  })
})

and I got the error like this
image

This is not the case when I use mount from cypress/vue

Adjustment

I adjusted your codes a bit and now the problem is gone. Here is the full function mount which I tried to follow from cypress/vue to make it consistent.

import { getContainerEl, injectStylesBeforeElement, setupHooks, type StyleOptions } from 'cypress/mount-utils';
import { html, LitElement, render, type TemplateResult } from 'lit';
import { unsafeHTML } from 'lit/directives/unsafe-html.js';

interface MountOptions<T extends HTMLElement> extends Partial<StyleOptions> {
    log?: boolean;
    properties?: Partial<T>;
}

let componentInstance: LitElement | HTMLElement | undefined;

Cypress.on('run:start', () => {
    if (Cypress.testingType !== 'component') {
        return;
    }

    Cypress.on('test:before:run', () => {
        componentInstance?.remove();
        const containerEl = getContainerEl();
        containerEl.innerHTML = '';
    });
});

export const mount = <T extends keyof HTMLElementTagNameMap>(
    template: TemplateResult | string,
    options: MountOptions<HTMLElementTagNameMap[T]> = {},
): Cypress.Chainable<JQuery<HTMLElementTagNameMap[T]>> => {
    return cy.then(() => {
        const containerEl = getContainerEl();

        injectStylesBeforeElement(options, document, containerEl);

        const elementTemplate = typeof template === 'string' ? html`${unsafeHTML(template)}` : template;
        const componentNode = document.createElement('div');
        componentNode.id = '__cy_lit_root';
        containerEl.append(componentNode);
        render(elementTemplate, componentNode);

        return cy
            .wrap(componentNode, { log: false })
            .wait(1, { log: false })
            .children()
            .first()
            .then((element) => {
                const name = element.prop('tagName').toLowerCase();
                const el = document.getElementsByTagName<T>(name)[0];
                const { properties, log } = options;

                if (properties && typeof properties === 'object' && Array.isArray(properties) === false) {
                    Object.entries(properties).forEach(([key, value]) => {
                        el[key as keyof typeof el] = value;
                    });
                }

                componentInstance = el;

                if (!log) {
                    const mountMessage = `<${name} ... />`;
                    Cypress.log({
                        name: 'mount',
                        message: [mountMessage],
                    })
                        .snapshot('mounted')
                        .end();
                }

                return cy.wrap(el, { log: false });
            });
    });
};

setupHooks();

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @ahnpnl , I think that is a better solution.

I guess because the element to mount is created then it is reliably there, even in before each, very neat.

I'll implement these along with the webpack example 👍

For the comment on just returning the component, I think consistency makes sense. So if this is how Vue command does it, then lit/web component should do the same 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

We are still figuring out the final specification/recommendation for Mount commands, but returning an object with a component property seems like a good idea - this way, if we need to add more properties on the return value in the future, we can do so in a non-breaking fashion.

properties?: Partial<T>
}

export type MountReturn<T extends keyof HTMLElementTagNameMap> =
Copy link
Author

Choose a reason for hiding this comment

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

Users should use the HTMLElementTagNameMap then all available properties of the underlying object can be accessed after mounting.

declare global {
  interface HTMLElementTagNameMap {
    "counter-lit": LitCounter;
    "counter-wc": WebCounter;
  }
}

@lmiller1990
Copy link
Contributor

lmiller1990 commented Oct 31, 2022

Hi @simonireilly, have you considered publishing just the npm/lit adapter on npm separately in the meantime? That way users could start using it immediately, all they'd need to do is import { mount } from '@simonireilly/lit-cypress' and they could start testing.. They wouldn't get the fully blown onboarding experience, but it would still be quite useful.

@simonireilly
Copy link
Author

Hi @simonireilly, have you considered publishing just the npm/lit adapter on npm separately in the meantime?

I can do this as a stop gap. All that's left for me to do in this PR is write a webpack config and a system test for that.

That way users could start using it immediately, all they'd need to do is import { mount } from '@simonireilly/lit-cypress' and they could start testing.

Sounds reasonable.

They wouldn't get the fully blown onboarding experience, but it would still be quite useful.

The cypress code throws a hard error for unsupported framework options, so users would need to set svelte or view
https://github.com/simonireilly/cypress-lit/blob/8b700c5a0068f530a7bb126a93c8364d85530ec1/cypress.config.ts#L6

It's probably a good idea to not throw an error in that instance, and allow users to write their own mount plugins.

This will also reduce the maintenance burden on cypress.

Forming an interface for framework plugins for the component runner should

  • not require cypress to maintain all plugins for all new UI frameworks
  • allow for the wizard to have an option of custom, still setting up basic file structure, but with no dependency checking

@lmiller1990
Copy link
Contributor

lmiller1990 commented Nov 1, 2022

The cypress code throws a hard error for unsupported framework options, so users would need to set svelte or view

Actually you should be able to do this right now without any changes to the main code using a custom dev server config: https://docs.cypress.io/guides/component-testing/component-framework-configuration#Component-Testing-Config.

There is also some examples:

For a more production like example, the Quasar Framework does various things, take a look https://github.com/quasarframework/quasar-testing/blob/dev/test-project-vite/cypress.config.ts#L27 (I don't know what they do exactly, but it works great).


I would like to have some kind of public API to let third parties inject a workflow into the onboarding wizard, but that would need to be considered carefully. This would be something we would do post Cypress 11 (which we are preparing now).


The best step forward to unlock people to use Lit right now would be:

  • publish lit mounting adapter on npm
  • in the README share example repo configuring devServer as shown above (so the onboarding wizard doesn't show up). I can help with this once the Lit adapter is available somewhere
  • write a bunch of examples, figure out any idioms or patterns needed for Lit in Cypress (if any)
  • have a good time testing with Lit!!!

WDYT?

@elylucas
Copy link
Contributor

elylucas commented Nov 1, 2022

Hi @simonireilly, have you considered publishing just the npm/lit adapter on npm separately in the meantime?

I can do this as a stop gap. All that's left for me to do in this PR is write a webpack config and a system test for that.

That way users could start using it immediately, all they'd need to do is import { mount } from '@simonireilly/lit-cypress' and they could start testing.

Sounds reasonable.

They wouldn't get the fully blown onboarding experience, but it would still be quite useful.

The cypress code throws a hard error for unsupported framework options, so users would need to set svelte or view https://github.com/simonireilly/cypress-lit/blob/8b700c5a0068f530a7bb126a93c8364d85530ec1/cypress.config.ts#L6

It's probably a good idea to not throw an error in that instance, and allow users to write their own mount plugins.

This will also reduce the maintenance burden on cypress.

Forming an interface for framework plugins for the component runner should

  • not require cypress to maintain all plugins for all new UI frameworks
  • allow for the wizard to have an option of custom, still setting up basic file structure, but with no dependency checking

Hi @simonireilly,

Another option we are pursing is setting up a new GitHub and NPM org for Cypress Community members to contribute plugins and new CT mounting libs such as this one. This will allow members to more easily find projects to use and contribute to that have a more official backing from Cypress, and allow you to have a place to host the plugin and "pass the torch" if you find you don't have the bandwidth to maintain it anymore. I thought your lit lib would be a great candidate for it.

Let me know what you think and if it sounds good we can probably kick this new program off with it.

Thanks

@simonireilly
Copy link
Author

simonireilly commented Nov 2, 2022

Hi @simonireilly, have you considered publishing just the npm/lit adapter on npm separately in the meantime?

I can do this as a stop gap. All that's left for me to do in this PR is write a webpack config and a system test for that.

That way users could start using it immediately, all they'd need to do is import { mount } from '@simonireilly/lit-cypress' and they could start testing.

Sounds reasonable.

They wouldn't get the fully blown onboarding experience, but it would still be quite useful.

The cypress code throws a hard error for unsupported framework options, so users would need to set svelte or view https://github.com/simonireilly/cypress-lit/blob/8b700c5a0068f530a7bb126a93c8364d85530ec1/cypress.config.ts#L6

It's probably a good idea to not throw an error in that instance, and allow users to write their own mount plugins.

This will also reduce the maintenance burden on cypress.

Forming an interface for framework plugins for the component runner should

  • not require cypress to maintain all plugins for all new UI frameworks
  • allow for the wizard to have an option of custom, still setting up basic file structure, but with no dependency checking

Hi @simonireilly,

Another option we are pursing is setting up a new GitHub and NPM org for Cypress Community members to contribute plugins and new CT mounting libs such as this one. This will allow members to more easily find projects to use and contribute to that have a more official backing from Cypress, and allow you to have a place to host the plugin and "pass the torch" if you find you don't have the bandwidth to maintain it anymore. I thought your lit lib would be a great candidate for it.

Let me know what you think and if it sounds good we can probably kick this new program off with it.

Thanks

@elylucas Yeah sounds good.

I've got fully tested source code in a separate repo so reach out and let me know 👍

@baus
Copy link

baus commented Nov 8, 2022

@simonireilly We appreciate your contributions to Cypress Component Testing.

Unfortunately we are unable to merge support for new frameworks directly into our main branch until we’ve finalized the API for exposing frameworks in the onboarding wizard. We also consider our ability to adequately support additional frameworks prior to merging mount() packages.

We do plan to provide the ability for outside contributors to build and maintain framework support as an external library.
We look forward to your feedback when we specify those capabilities.

Thanks again,
Chris Baus, Cypress Component Testing

@simonireilly
Copy link
Author

@simonireilly We appreciate your contributions to Cypress Component Testing.

Unfortunately we are unable to merge support for new frameworks directly into our main branch until we’ve finalized the API for exposing frameworks in the onboarding wizard. We also consider our ability to adequately support additional frameworks prior to merging mount() packages.

We do plan to provide the ability for outside contributors to build and maintain framework support as an external library.
We look forward to your feedback when we specify those capabilities.

Thanks again,
Chris Baus, Cypress Component Testing

Thanks for the response.

I totally respect that. I would think your time is best spent on ensuring your core product works, and creating an API for expansion.

I'll consider making this a standalone package.

I'll also close this PR 👍

@lmiller1990
Copy link
Contributor

lmiller1990 commented Nov 8, 2022

There's some info/discussion on how to make the mount adapter a standalone package here: #24494 (comment)

We have requests for Solid.js support, too -- going to start thinking more about how to make a solid public API so third parties can author CT frameworks without needing to make changes to the main code base.

@simonireilly
Copy link
Author

To be continued on this repo: https://github.com/simonireilly/cypress-lit

Thanks everyone for their time and input 👍

@lmiller1990
Copy link
Contributor

lmiller1990 commented Feb 17, 2023

Ho @simonireilly, we have a public API for this feature coming in the next release.

If either of you is interested, you could either make your own one for Lit now. It would be mostly copy+pasting https://github.com/simonireilly/cypress-lit and adding a few things. If you want to work on it together or have any questions, please let me know.

@simonireilly simonireilly deleted the feat-lit-support branch February 17, 2023 07:21
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.

7 participants