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

fix: don't throw error for tagName getter in Jest #3269

Closed
wants to merge 1 commit into from

Conversation

nolanlawson
Copy link
Collaborator

@nolanlawson nolanlawson commented Jan 6, 2023

Details

Fixes salesforce/sfdx-lwc-jest#299

I believe that, long-term, we should do #3245 and eliminate functional differences between dev mode and prod mode. However, that is a larger effort, whereas this is a quick fix.

It is currently not possible to write a test for this, because we don't run any engine-dom or engine-core tests in Jest.

Does this pull request introduce a breaking change?

  • ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ✅ No, it does not introduce an observable change.

@jye-sf
Copy link
Contributor

jye-sf commented Jan 6, 2023

We have a karma test for this at https://github.com/salesforce/lwc/tree/master/packages/%40lwc/integration-karma/test/component/LightningElement.tagName. May need to update that, and you could test this new functionality there.

@nolanlawson
Copy link
Collaborator Author

@jye-sf The karma tests don't run with process.env.NODE_ENV === 'test' enabled. That is only used for Jest.

@nolanlawson
Copy link
Collaborator Author

/nucleus test

@@ -268,6 +268,11 @@ function getCustomElementRestrictionsDescriptors(elm: HTMLElement): PropertyDesc

function getComponentRestrictionsDescriptors(): PropertyDescriptorMap {
assertNotProd(); // this method should never leak to prod
if (process.env.NODE_ENV === 'test') {
Copy link
Contributor

@ravijayaramappa ravijayaramappa Jan 6, 2023

Choose a reason for hiding this comment

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

I tried this change with the reported test case, the new assertion message is

    expect(jest.fn()).not.toBeCalled()

    Expected number of calls: 0
    Received number of calls: 1

    1: {Symbol(@@lockerLiveValue): undefined}

      10 |         document.body.appendChild(element);
      11 |         debugger;
    > 12 |         expect(foo).not.toBeCalled();
         |                         ^
      13 |     });
      14 | });

      at Object.toBeCalled (force-app/main/default/lwc/myComponent/__tests__/index.spec.js:12:25)

The error message isn't much useful to determine that the value is an LWC Component. Jest serializes it like an Object and prints out the enumerable properties. This is acceptable.
However, if we want to make it a bit better, IMO the solution is to provide a serializer for LightningElement component instance via @lwc/jest-serializer and have it call the LightningElement.toString() method

The background is that jest tries to detect the value type. For DOMElement, it uses the tagName property to test if the value is a DOMElement. Hence the error reported by the user. The code is in pretty-format package.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

However, if we want to make it a bit better, IMO the solution is to provide a serializer for LightningElement component instance via @lwc/jest-serializer and have it call the LightningElement.toString() method

This makes sense; we can open an issue on lwc-test for that.

AIUI, if we did this, then we would not need to add any code to LWC OSS to do special handling for Jest. So yeah, that makes sense to me as an alternative.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Jest throws error because of LWC restriction on accessing tagName
3 participants