DevExpress / testcafe-hammerhead

A powerful web-proxy used as a core for the TestCafe testing framework. :hammer: :smiley:
https://testcafe.io
MIT License
174 stars 161 forks source link

Fix null window case in _getCurrentInfoManager() #2936

Closed bancer closed 1 year ago

bancer commented 1 year ago

Purpose

This is a fix for an issue reported in https://github.com/DevExpress/testcafe/issues/7886. You can find the stacktrace there.

Approach

I do not know if this is the correct approach but it is logical and it fixes the problem. I tested it by editing hammerhead.js directly in our vendor folder. The reasoning: The use case here is when contextWindow value is undefined what makes no need to call getSandboxBackup(). getSandboxBackup() aka get() called at the bottom of _getCurrentInfoManager() function may return null according to https://github.com/DevExpress/testcafe-hammerhead/blob/6607246aca35d9b1f5f3ef1523513641a5b1aefd/src/client/sandbox/backup.ts#L44-L56 but null case was not processed in _getCurrentInfoManager(). So I assume that if it is okay for getSandboxBackup() to return null then it should also be okay for _getCurrentInfoManager() to return null.

I cannot add unit tests as my knowledge of hammerhead is very limited.

Debug for the context:

Screenshot from 2023-07-19 15-14-26

Screenshot from 2023-07-19 15-14-46

References

https://github.com/DevExpress/testcafe/issues/7886

Pre-Merge TODO

miherlosev commented 1 year ago

Hi @bancer,

Thank you for this information. This behavior is incorrect. The contextWindow should not be equal to null. However, we cannot accept your PR. It's difficult to determine the cause of this behavior without a simple example. Please share a sample project that illustrates the behavior.

bancer commented 1 year ago

Should that be something like this then?

        if (!contextWindow) {
            throw new Error('Context window is undefined');
        }
miherlosev commented 1 year ago

Should that be something like this then?

No. We need to fix the flow that leads to the contextWindow === null situation. This is why we need a reproducible example.

andrzej-woof commented 1 year ago

@miherlosev I believe I can get you a way to reproduce it. Navigating to this URL in testcafe with native automation seems to cause same exception https://react-dropzone.js.org/#section-basic-example

image

ayemelyanenko-chegg commented 1 year ago

Sorry to chime in but does the testcafe log help in reproducing?

https://upload.disroot.org/r/1x3h34w8#bdMOCO33y7ltfgPrMsgsN9EyQcDBqSfc2pBzA6FApbk=

miherlosev commented 1 year ago

@ayemelyanenko-chegg

Thank you for the information. I already have an example to reproduce this behavior. See the latest updates at https://github.com/DevExpress/testcafe/issues/7886.