fix(transfer): fix transfer undefined imports when testing

[ghost]

See https://dhis2.atlassian.net/browse/LIBS-342.

I've added two tests. One that uses the untranspiled code and succeeds, and one that uses the transpiled code and fails. I've narrowed it down to an issue with styled-jsx. It seems the cause of the problem is linked to this:

https://github.com/dhis2/app-platform/blob/ede728c7a1e8ca84940b580917b80cfd2eaf5b33/cli/config/makeBabelConfig.js#L56-L70

If you look at our babel config there, you'll see that styled-jsx is not compiling styles for test environments, as recommended in their docs: https://github.com/vercel/styled-jsx#rendering-in-tests. That's the right approach to prevent any errors in jest, see this issue for example: vercel/styled-jsx#516 (there are a lot of others like it), and vercel/styled-jsx#469 (comment).

So the above babel config fixes any issues with jest when you allow jest to transpile the code with the correct babel config. However, if you aren't testing a local module, but a module imported from node_modules, you're getting a module that has been transpiled for production and so that seems to fail with jest again. So that seems to be where it's going wrong.

To be more precise. It seems that the styled-jsx transform will flag expressions it can't evaluate at build time as dynamic: vercel/styled-jsx#595 (comment). So these imports in the transfer are one such case for example. Part of the transfer transpiles down to this:

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.LeftSide = void 0;

var _style = _interopRequireDefault(require("styled-jsx/style"));

var _propTypes = _interopRequireDefault(require("prop-types"));

var _react = _interopRequireDefault(require("react"));

var _index = require("./common/index.js");

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

const LeftSide = _ref => {
  let {
    children,
    dataTest,
    width
  } = _ref;
  return /*#__PURE__*/_react.default.createElement("div", {
    "data-test": dataTest,
    className: _style.default.dynamic([["4143353612", [_index.backgroundColor, _index.borderRadius, _index.borderColor, width]]])
  }, children,
  /**
   * Flex basis 0px to make sure right and left side
   * always have the same width
   */
  '', /*#__PURE__*/_react.default.createElement(_style.default, {
    id: "4143353612",
    dynamic: [_index.backgroundColor, _index.borderRadius, _index.borderColor, width]
  }, ["div.__jsx-style-dynamic-selector{display:-webkit-box;display:-webkit-flex;display:-ms-flexbox;display:flex;-webkit-flex-direction:column;-ms-flex-direction:column;flex-direction:column;background-color:".concat(backgroundColor, ";border-radius:").concat(borderRadius, ";border:1px solid ").concat(borderColor, ";min-height:240px;max-width:100%;width:").concat(width, ";}")]));
};

exports.LeftSide = LeftSide;
LeftSide.propTypes = {
  width: _propTypes.default.string.isRequired,
  children: _propTypes.default.node,
  dataTest: _propTypes.default.string
};

and as you can see, backgroundColor is referred to but not defined in this module (see the part under dynamic). I didn't look into it any further than that. So why exactly styled-jsx isn't failing in the browser and only in jest I'm not sure. The code is not necessarily invalid, i.e. this is valid:

so assuming that styled-jsx uses something like a tagged template to evaluate this, it's probably that that's going wrong in jest somehow, maybe it's due to jsdom.

1. One potential fix is just to not test any ui components in apps in jest. Which makes sense because they're already tested in ui, there's no need to duplicate that unless you're integration testing (and we use cypress for that currently, which has no problem with styled-jsx). You can just use shallow rendering and assume that the component does what it should.
2. Another solution is to fix the import style used in the transfer. Importing directly from the constants package instead of reexporting seems to fix the issue as well. Apparently that doesn't get flagged as dynamic. Maybe there are other solutions as well, but I can't think of any at the moment.

Anyway, it seems that this is just a downside of using styled-jsx the way we use it. Apparently baking styled-jsx into our published modules also bakes in the jest incompatibility. Let me know if I've overlooked/misinterpreted anything!

[dhis2-bot]

🚀 Deployed on https://pr-1137--dhis2-ui.netlify.app

[cypress]

---

Test summary

558 • 0 • 0 • 0

---

Run details

Project	ui
Status	Passed
Commit	116978d
Started	Sep 1, 2022 9:01 AM
Ended	Sep 1, 2022 9:09 AM
Duration	08:01 💡
OS	Linux Ubuntu - 20.04
Browser	Electron 94

View run in Cypress Dashboard ➡️

---

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

[HendrikThePendric]

Thanks for looking into this @ismay your analysis and explanation make a lot of sense. There is one thing I still don't fully understand, and it is relates to this:

One potential fix is just to not test any ui components in apps in jest. Which makes sense because they're already tested in ui, there's no need to duplicate that unless you're integration testing (and we use cypress for that currently, which has no problem with styled-jsx). You can just use shallow rendering and assume that the component does what it should.

I've copied your entire point and highlighted the part I find a bit confusing. You are suggesting that using Enzyme's shallow render would prevent this issue from happening. This makes sense to me as well, but I seem to remember trying this in the scheduler-app and I'm pretty sure I was still getting the same error. So this part I still don't really get.

But, thinking about the shallow() VS mount() stuff some more, I'm not sure that being restricted to only using shallow() is a workable solution. Suppose:

• You have built a custom component in an app
• This component renders a component from UI
• To test the custom component you need to mount() it for whatever reason

Then I think things would still break. And you'd need to mock the UI component instead...

Having said that, I guess there is not much we can do about this type of behaviour. Unless we fundamentally revisit our tooling and/or publishing setup....

[ghost]

Thanks for looking into this @ismay your analysis and explanation make a lot of sense. There is one thing I still don't fully understand, and it is relates to this:

One potential fix is just to not test any ui components in apps in jest. Which makes sense because they're already tested in ui, there's no need to duplicate that unless you're integration testing (and we use cypress for that currently, which has no problem with styled-jsx). You can just use shallow rendering and assume that the component does what it should.

I've copied your entire point and highlighted the part I find a bit confusing. You are suggesting that using Enzyme's shallow render would prevent this issue from happening. This makes sense to me as well, but I seem to remember trying this in the scheduler-app and I'm pretty sure I was still getting the same error. So this part I still don't really get.

Yeah the wording is a bit confusing maybe. What I meant is that you can assume that the component from @dhis2/ui is doing what it should. Since that's already tested in ui. The enzyme docs phrase it well:

Shallow rendering is useful to constrain yourself to testing a component as a unit, and to ensure that your tests aren't indirectly asserting on behavior of child components. (https://enzymejs.github.io/enzyme/docs/api/shallow.html#shallow-rendering-api)

So any child components are stubbed and don't factor into the test. Which seems like a sound solution to me as we use jest for unit tests. As to why it didn't work in the scheduler, I'm not sure. We should take a look at that.

You have built a custom component in an app
This component renders a component from UI
To test the custom component you need to mount() it for whatever reason

The docs for mount state a couple usecases:

Full DOM rendering is ideal for use cases where you have components that may interact with DOM APIs or need to test components that are wrapped in higher order components.

Full DOM rendering requires that a full DOM API be available at the global scope. This means that it must be run in an environment that at least “looks like” a browser environment. If you do not want to run your tests inside of a browser, the recommended approach to using mount is to depend on a library called jsdom which is essentially a headless browser implemented completely in JS. (https://enzymejs.github.io/enzyme/docs/api/mount.html)

So yeah there might be a couple scenarios in which you'd want to use mount. We'd have to work around those. I don't think that's too bad. We always have cypress, and these scenarios don't necessarily have to overlap with components that use ui components. Plus if they do the ui components can be mocked, then you should still be able to use mount. That would be like a kind of manual shallow rendering.

[ghost]

I'm wondering if a third potential solution would be to mock styled-jsx/style in such a way that it wouldn't crash where it's currently failing. That might be possible as well. I'd have to look into that.

[DNR800]

Thanks for looking at this @ismay. It sounds like we might run into this problem in a few other places at some point too given its a problem with styled-jsx/style so I think its good to think about ways to navigate around this problem  👍

I think from memory that LeftSide and RightSide were basically sharing some values via a local module - most of which were constants so I don't think i would object to solution 2. I think only one value might be duplicated but I think as a couple of developers have run in to problems testing this component having the component in a better state for testing would be beneficial.

When I ran in to the problem I was working in the Schedule App and was create a custom form field that I was looking to test. Although the field was pulling in data from the server it felt like jest was a good fit for it as it seemed like a relatively small piece of code. I generally find jest tests quicker to write and I like that they tend to be quick in giving feedback - saying that I haven't really spun up the Cypress tests locally yet so I should probably look at doing that too (my bad)

I think there might be some merit to what you say about a potential third solution but I don't really know how complex mocking styled-jsx/style for the tests would be - maybe we would need a spike around it. I guess another short term solution might be that if we come across this again in using the UI lib components we could mock out that component specifically. Its not something I would like to do a lot of but if we don't care about the specific implementation of that component in our test It think that would be fine - in some way it might actually give use some understanding of how often we come around this problem in future too.

[ghost]

I'm wondering if a third potential solution would be to mock styled-jsx/style in such a way that it wouldn't crash where it's currently failing. That might be possible as well. I'd have to look into that.

I've tried, but I'm not able to make this work. It seems like the error isn't occuring in styled-jsx but in react.createElement, which is evaluating the expression without having the necessary values in scope. So 3 doesn't seem viable.

[ghost]

Thanks for looking at this @ismay. It sounds like we might run into this problem in a few other places at some point too given its a problem with styled-jsx/style so I think its good to think about ways to navigate around this problem +1

My pleasure 👍️. Yeah I agree, it's likely that this'll happen with other components as well.

I think from memory that LeftSide and RightSide were basically sharing some values via a local module - most of which were constants so I don't think i would object to solution 2. I think only one value might be duplicated but I think as a couple of developers have run in to problems testing this component having the component in a better state for testing would be beneficial.

When I ran in to the problem I was working in the Schedule App and was create a custom form field that I was looking to test. Although the field was pulling in data from the server it felt like jest was a good fit for it as it seemed like a relatively small piece of code. I generally find jest tests quicker to write and I like that they tend to be quick in giving feedback - saying that I haven't really spun up the Cypress tests locally yet so I should probably look at doing that too (my bad)

Yeah that's right, it's the imported values that the transfer is using that are the problem. Outside of not using styled-jsx I think our most viable solution is to not mount ui components in jest tests outside of the ui lib (so solution 1).

It's safe to assume that ui components are doing what they should for your jest tests. So feel free to mock them for your jest tests if they're causing issues. And if you want to write an integration test cypress is more realistic and works with our ui components.

So outside of any changes in our preferences for the framework to use for integration tests i think that would be my recommended way forward. Not ideal, but it seems the most practical to me. We can of course still look into 2., but in terms of a testing strategy I'd go with 1.

[DNR800]

Thanks for looking into this Ismay. From what you say there it does look like 1 is the best option

[ghost]

Thanks for looking into this Ismay. From what you say there it does look like 1 is the best option

Anytime 👍️. Yeah it's not ideal and maybe there's a better workaround, but I couldn't find anything that worked.

[HendrikThePendric]

@ismay

One potential fix is just to not test any ui components in apps in jest

I agree we shouldn't test any UI components in our apps. But there are quite a few valid scenarios where we do have mount (or shallow-mount) these components. Some basic examples that come to mind:

• Checking if the correct options are rendered
• Checking if the onChange callback has the expected effects (i.e. other components may change too?)

Mocking UI components all the time seem rather inconvenient.

Not quite sure what we want to do with this PR BTW. Having this test for 1 single component doesn't seem to make sense. Having it for all components seems a bit odd too. Perhaps we can simply close it?

[ghost]

Yeah no the tests were just a demonstration. In the end what we're running into here is a technical limitation of styled-jsx it seems. It seems you can't use the components that were compiled for production in jest, in certain conditions. I've not had the time to get completely to the bottom of this, because it concerns both bundling and babel, which gets very complex.

I think the most practical way forward for now is to fix the import style of the transfer, as it seems that that's somehow causing this issue (still not sure how). Most of our other components are fine in jest.

Also, I still think devs shouldn't be testing internals of our ui components in any way, that's our job. They'll need them to run properly in integration tests of course (and they do), but they shouldn't be asserting anything that's our responsibility, that'll just couple their tests unnecessarily to our implementation. So I'd say this is a low priority bug for that reason, if a very annoying one.