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

Cannot use both mocked realm and actual realm in a test #6122

Open
Yupeng-li opened this issue Sep 8, 2023 · 13 comments
Open

Cannot use both mocked realm and actual realm in a test #6122

Yupeng-li opened this issue Sep 8, 2023 · 13 comments

Comments

@Yupeng-li
Copy link

Yupeng-li commented Sep 8, 2023

How frequently does the bug occur?

Always

Description

If I mock realm in a test file, and then import the real realm in a particular test, it will give this error:

{{ TypeError: Cannot redefine property: Realm}}

The error is from this line. Object.defineProperty was called twice.

To reproduce the issue, see example below.

jest.mock('realm')

it('a test', () => {
    const Realm = jest.requireActual('realm')
    // ...
})

Stacktrace & log output

No response

Can you reproduce the bug?

Always

Reproduction Steps

No response

Version

12.1.0

What services are you using?

Local Database only

Are you using encryption?

No

Platform OS and version(s)

Mac OS 13.5.1 (22G90)

Build environment

Which debugger for React Native: ..

Cocoapods version

1.12.1

@kneth
Copy link
Contributor

kneth commented Sep 8, 2023

@Yupeng-li Mocking Realm in Jest isn't trivial. You might want to seek inspiration in #370 (comment)

@sync-by-unito sync-by-unito bot added the Waiting-For-Reporter Waiting for more information from the reporter before we can proceed label Sep 8, 2023
@Yupeng-li
Copy link
Author

Hi @kneth , thank you! My issue isn't about how to mock realm, but realm module throws when it's imported multiple times. The sample code above shows the scenario when it could be imported twice. Our use case is slightly different. We use a jest custom matcher which imports Realm. Our tests also import realm, so the module is imported twice.

I temporarily fixed the issue I reported but then I got errors like:
Error: Expected value to be an instance of List, got an instance of List
Error: Illegal constructor: Results objects are read from managed objects only.

It seems that assertion is checking instance type against two copies of the module. We didn't have the issue with Realm v11, so I am wondering if this is something you could fix?

Best regards

@github-actions github-actions bot added Needs-Attention Reporter has responded. Review comment. and removed Waiting-For-Reporter Waiting for more information from the reporter before we can proceed labels Sep 8, 2023
@Yupeng-li
Copy link
Author

There is a more common scenario. If I mock a file that imports Realm, the TypeError will be thrown. Please let me know if you need a min repo.

// functionB.ts

import Realm from 'realm' 

export const functionB = () => {
    console.log('function B', typeof Realm)
}
// some.test.ts
import Realm from 'realm' // this is required to reproduce the issue 
import { functionB } from './functionB'

jest.mock('./functionB') // This crashes the test because of the error: TypeError: Cannot redefine property: Realm

it('a test', () => {
    console.log('ss', typeof Realm) // This could be the code pre-populating data to realm, e.g. realm.create(...)
    functionB()
})

@a396901990
Copy link

I have the same issue, the test stopped working since upgrading to realm v12.3.0

@kraenhansen kraenhansen self-assigned this Feb 12, 2024
@sync-by-unito sync-by-unito bot removed the Needs-Attention Reporter has responded. Review comment. label Feb 12, 2024
@liamjones
Copy link

Hi @kraenhansen I'm going through a bunch of upgrades on our apps atm (RN, 3rd-party deps, etc) and I was just wondering if there was any news on this issue yet? It's our remaining blocker to upgrade from Realm 11 to 12.

@liamjones
Copy link

Hi @kraenhansen, you mentioned before that you thought this might be a quick thing to fix.

With Realm going EOL and you working on other projects at Mongo I understand you might never get a chance to look at fixing this now.

Even if you can't fix this yourself, can you point us in the right direction in the codebase so we can look at patching this ourselves?

@kraenhansen
Copy link
Member

Looking into this, I've actually already done the change (it was included in this huge refactor) and if it was what I thought it was, it should be fixed in v12.6.1.

Notice how the Realm property is defined as configurable: false in the old code:

Object.defineProperty(safeGlobalThis, "Realm", {
get() {
if (flags.THROW_ON_GLOBAL_REALM) {
throw new Error(
"Accessed global Realm, please update your code to ensure you import Realm:\nimport Realm from 'realm';",
);
} else if (!warnedAboutGlobalRealmUse) {
// eslint-disable-next-line no-console
console.warn(
"Your app is relying on a Realm global, which will be removed in realm-js v13, please update your code to ensure you import Realm:\n\n",
'import Realm from "realm"; // For ES Modules\n',
'const Realm = require("realm"); // For CommonJS\n\n',
"To determine where, put this in the top of your index file:\n",
`import Realm from "realm";\n`,
`Realm.flags.THROW_ON_GLOBAL_REALM = true`,
);
warnedAboutGlobalRealmUse = true;
}
return RealmConstructor;
},
configurable: false,
});

The code was moved and now it's configurable: true:

Can you verify that this has been resolved and if not, provide an updated stacktrace?

I'm sorry I didn't realize that we could close this issue based on that refactoring PR - a lot of things went into that 😬

@liamjones
Copy link

liamjones commented Nov 28, 2024

Looking into this, I've actually already done the change (it was included in #6492) and if it was what I thought it was, it should be fixed in v12.6.1.

Oh, that's a pleasant surprise!

Can you verify that this has been resolved and if not, provide an updated stacktrace?

Sure, this was blocking us moving from realm 11 to 12 previously so I'll have to resurrect the 11->12 changes we needed to make but I'll let you know. Many thanks!

@vinibgoulart
Copy link

There is this example for realm mock in jest, its working for me as far as possible. Maybe can help someone

https://gist.github.com/hyb175/beb9ceed4c34300ba7c77d3d6d44ae52

@liamjones
Copy link

liamjones commented Dec 3, 2024

Hi again @kraenhansen,

I'm trying out 12.14.0 and you're right, the "Cannot redefine property: Realm" problem is gone. 🥳

But the other issues Yupeng mentioned remain. 😢

I've pulled together a minimal repro showing off one of them (hoping that both have the same cause), would you be able to take a look?

  1. Clone https://github.com/liamjones/RealmMockIssue
  2. npm install
  3. npm run test
  4. Test will fail with Illegal constructor: Results objects are read from managed objects only. and Jest doesn't exit
  5. Comment out the mocking of the file that has an import from Realm inside it: https://github.com/liamjones/RealmMockIssue/blob/main/__tests__/realm.test.ts#L6
  6. npm run test
  7. Test passes but Jest still doesn't exit

@kraenhansen
Copy link
Member

kraenhansen commented Dec 3, 2024

Erhm. I've spent quite a bit of time trying to understand what's going on there - unfortunately without luck this far.

Jest's automock feature seems very powerful (and intrusive).
The meat of the issue is, that Jest (somehow - unbeknownst to me) manages to break an internal invariant of the SDK.

For some reason, the binding.Results bound as constructor for the instance returned by binding.Results.fromTable here:

const results = binding.Results.fromTable(internal, table);

is not the same as binding.Results within Results.ts:

if (arguments.length === 0 || !(internal instanceof binding.Results)) {

internal.constructor.name === binding.Results.name // true
internal.constructor === binding.Results // false
internal instanceof binding.Results // false

I have no clue as to why this happens and I don't understand why Jest even touch the binding.Results or binding.Results.fromTable in the first place - is the jest.mock('../src/hasRatedThisVersion.ts') call transitive somehow?

@liamjones
Copy link

Thanks for taking a look @kraenhansen.

The fact that it's the automock process breaking this is interesting.

As far as I understand it, automocking does 'execute' the module being mocked to work out its exports shape. I would have thought it'd only have an impact on the directly mocked module, not any of the transitive dependencies but apparently not...

At a guess, I suspect it might be down to this code where Jest transforms the hasRatedThisVersion code, and uses the Node vm module to create a context and run the module script within it. My guess is that the Results object created in the alternate context is somehow leaking out of the mock-making process for the hasRatedThisVersion module and then the instanceof doesn't match up later.

But this is all guess work, maybe I need to raise an issue with the Jest project, they might have a better idea of what's going on.

The fact that it's the automock code specifically causing the issue does give us a workaround though. If we manually mock by providing a factory function:

jest.mock('../src/hasRatedThisVersion.ts', () => { hasRatedThisVersion: jest.fn() });

Then the automocking code is skipped and things work fine. It's slightly annoying having to maintain a manual copy of the module's exports but it's not the end of the world if we can't do anything more.

@kraenhansen
Copy link
Member

kraenhansen commented Dec 5, 2024

It's slightly annoying having to maintain a manual copy of the module's exports but it's not the end of the world if we can't do anything more.

For what it's worth, I noticed jest.mock takes a type parameter and although a bit verbose, you can leverage that to gain type-safety:

jest.mock<typeof import('../src/hasRatedThisVersion.ts')>('../src/hasRatedThisVersion.ts', () => {
  return { hasRatedThisVersion: jest.fn<() => boolean>() };
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants