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

[Bug]: Test breaks after upgrading to v9.18.3 #27720

Closed
2 tasks done
flora8984461 opened this issue Apr 27, 2023 · 8 comments
Closed
2 tasks done

[Bug]: Test breaks after upgrading to v9.18.3 #27720

flora8984461 opened this issue Apr 27, 2023 · 8 comments

Comments

@flora8984461
Copy link
Contributor

flora8984461 commented Apr 27, 2023

Library

React Components / v9 (@fluentui/react-components)

System Info

System:
    OS: Windows 10 10.0.22621
    CPU: (12) x64 Intel(R) Xeon(R) W-2133 CPU @ 3.60GHz
    Memory: 51.85 GB / 63.57 GB
  Browsers:
    Edge: Spartan (44.22621.1105.0), Chromium (109.0.1518.78)
    Internet Explorer: 11.0.22621.1

Are you reporting Accessibility issue?

None

Reproduction

https://github.com/flora8984461/reproduce-jest-failure/tree/master

Bug Description

I found a similar issue #5971 but it's for different versions of fluentui.

Actual Behavior

Before we use v9.11.1, and the test runs well. After upgrading to v9.19.1, it breaks with the error:

    TypeError: Cannot redefine property: useIsOverflowItemVisible
        at Function.defineProperty (<anonymous>)

Not only for useIsOverflowItemVisible, also happens in other utils from overflow, such as useOverflowMenu. I haven't checked if other libraries have the same issue.

Expected Behavior

The test should pass.

Reproduce steps

  1. Clone the repo and run yarn and yarn test.
    Note that the test will get error:
    TypeError: Cannot redefine property: useIsOverflowItemVisible at Function.defineProperty ().

  2. Change the version of fluentui/react-components to 9.11.1 in package.json, and run yarn and yarn test.
    Note that the test will pass.

  3. It also happens with other utilities from overflow, such as useOverflowMenu.

Logs

No response

Requested priority

High

Products/sites affected

No response

Are you willing to submit a PR to fix?

yes

Validations

  • Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • The provided reproduction is a minimal reproducible example of the bug.
@flora8984461
Copy link
Contributor Author

I narrowed down the version when it breaks, it's v9.18.3, suspect with this change #27250

There's issue about Jest with swc swc-project/swc#5059

@flora8984461 flora8984461 changed the title [Bug]: Test breaks after upgrading to v9.19.1 [Bug]: Test breaks after upgrading to v9.18.3 Apr 27, 2023
@Hotell Hotell assigned TristanWatanabe and unassigned Hotell Apr 28, 2023
@layershifter
Copy link
Member

Fixture

// @ts-expect-error
export { X } from "x";

Output TypeScript

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.X = void 0;
// @ts-expect-error
var x_1 = require("x");
Object.defineProperty(exports, "X", {
  enumerable: true,
  get: function() {
    return x_1.X;
  }
});

image

Output SWC

// @ts-expect-error
"use strict";
Object.defineProperty(exports, "__esModule", {
  value: true
});
Object.defineProperty(exports, "X", {
  enumerable: true,
  get: function() {
    return _x.X;
  }
});
const _x = require("x");

image


Is it SWC issue? It's questionable TBH:

If native esm mode fails, jest.spyOne working with ts-jest is simply a bug of tsc.
Closing as swc is doing the correct thing.

swc-project/swc#3843 (comment)

As workaround on our side we can look to use jest_workaround (swc-project/swc#5151 (comment)). But I don't think that it's a good idea.

Anyway @TristanWatanabe will look into it and provide more details.

How to solve it on consumer side now?

jest.mock("@fluentui/react-components", () => {
  return {
    ...jest.requireActual("@fluentui/react-components"),
    useIsOverflowItemVisible: jest.fn()
  };
});

import React from "react";
import { render, screen } from "@testing-library/react";
import { useIsOverflowItemVisible } from "@fluentui/react-components";
import { OverflowMenuItem } from "./ComponentToTest";

describe("Component to test", () => {
  it("Should not render hidden item", () => {
    (useIsOverflowItemVisible as jest.Mock).mockImplementation(() => false);

    const { container } = render(<OverflowMenuItem id="2" />);
    expect(container).toMatchSnapshot();
    const button = screen.queryByRole("button");
    expect(button).toBeNull();
  });
});

@flora8984461 you can use jest.mock & jest.requireActual to update your tests, the pattern is above.

@layershifter
Copy link
Member

To defend SWC, Babel does the same:

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
Object.defineProperty(exports, "X", {
  enumerable: true,
  get: function () {
    return _x.X;
  }
});
var _x = require("x");

image

@ling1726
Copy link
Member

ling1726 commented May 2, 2023

It seems that TS works because the exports.X = void 0 (i.e. normal assignment) is the same as calling Object.define with configurable: true https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty

@flora8984461
Copy link
Contributor Author

Thank you all for your quick help! 👍It seems an issue on Jest side. Found an open issue jestjs/jest#9430

So we will use mock and requireActual in future tests for components using fluentui.

You can close this issue or leave it for documentation purpose?

@Hotell
Copy link
Contributor

Hotell commented May 3, 2023

Thank you all for your quick help! 👍It seems an issue on Jest side. Found an open issue jestjs/jest#9430

@flora8984461

not sure if that's related, your jest is not consuming native ESM rather transpiled commonjs code

@Hotell
Copy link
Contributor

Hotell commented May 3, 2023

using jest.mock & jest.requireActual

I don't have proper context how you test your code but this feels like a brittle test/contract within your codebase.

I'd encourage you to rather focus on using real browser tests via playwright or cypress instead of mocking UI apis under the hood.

From fluent API perspective we cannot guarantee that your mocks will work with new releases as you'r testing functionality that is out of our control.

@TristanWatanabe
Copy link
Member

Thanks for looking into this @layershifter and @ling1726! Will close the issue now since this seems to be a jest issue - can revisit this thread if more people run into this.

@microsoft microsoft locked as resolved and limited conversation to collaborators Jun 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants