Skip to content

Fix for issues_657 and issue_983 #989

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

Closed
wants to merge 1 commit into from

Conversation

P-Courteille
Copy link

@P-Courteille P-Courteille commented Jul 13, 2025

Hello

📝 Description

Attempt to fix:


💡 Explanation

In the previous version of the code, the OCR function was added first to each instance of the browser and then to the browser object.

The problem is, @wdio\utils\build\index.js (line 388) already add the command to each instance so the commande added to each instance was overwritten by the one containing the loop of each instance.

→ Resulting in the issue 983

This issue was also the reason of the loop from issue 657, because each instance called the multi remote version of the command when we use:
browserInstance[commandName].apply(browserInstance, args)

✏️ Modifications

  1. Tried to update the type to be compatible with the change done in this webdriverio commit, without this change, the build was not possible.

→ I'm not really confident about this part

  1. Change the addCommand order for Multi Remote

    • first, add the command to Browser in a Multi Remote version (with a loop to do the function in each instance)
    • then, add the command to each instance in a Single Remote version
      In both ocr-service and visual-service
  2. Inside the package.json of visual reporter:
    "clean": "rimraf coverage build *.tsbuildinfo"
    is replaced by:
    "clean": "rimraf coverage build --glob *.tsbuildinfo"
    Because without that, it will fail on Windows in pathArg check :

    if (platform === 'win32') {
        const badWinChars = /[*|"<>?:]/;
        const { root } = parse(path);
        if (badWinChars.test(path.substring(root.length))) {
            throw Object.assign(new Error('Illegal characters in path.'), {
                path,
                code: 'EINVAL',
            });
        }
    }

⚠️ Warnings

I'm not a javascript/typescript expert so maybe:

  • it's overkill,
  • some changes are useless,
  • some changes are perfectible,
  • some changes doesn't follow good practice

But at least it work with both calls from Browser or MultiRemoteBrowser and no longer loop.


Best regard,
Paul

Copy link

changeset-bot bot commented Jul 13, 2025

🦋 Changeset detected

Latest commit: 9fe32a8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@wdio/ocr-service Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

linux-foundation-easycla bot commented Jul 13, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@P-Courteille P-Courteille changed the title Fix for issues_657 and issue_983 (ocr-service scope only) Fix for issues_657 and issue_983 Jul 16, 2025
@wswebcreation
Copy link
Member

Hi @P-Courteille

Thanks for this PR. I'm currently in the middle of a large refactor that would "simplify" a lot for the Visual package. See also #982. I want to release this in the coming days.

Can you do me a favor? First of all split all adjustments into "smaller" PR's that are connected to each service separately. So we can release them as separate fixes?

It would be great if you can create a PR for the Visual Service based on #982 (I know, a big ass one). But then we can release it as one fix.

Thanks again!

@P-Courteille
Copy link
Author

P-Courteille commented Jul 19, 2025

Hi @P-Courteille

Thanks for this PR. I'm currently in the middle of a large refactor that would "simplify" a lot for the Visual package. See also #982. I want to release this in the coming days.

Can you do me a favor? First of all split all adjustments into "smaller" PR's that are connected to each service separately. So we can release them as separate fixes?

It would be great if you can create a PR for the Visual Service based on #982 (I know, a big ass one). But then we can release it as one fix.

Thanks again!

Hi @wswebcreation,

I'm not sure I've correctly understood what you expect from me (especially for the base branch of fixes for other packages than visual-service), so if I miss the point, don't hesitate to let me know.

I retrieved your PR 982, ws/rewrite-core and then:

  • Created PR 991 using ws/rewrite-core as base to fix issue 990 for visual-reporter
  • Created PR 992 using ws/rewrite-core as base to fix issue 657 for ocr-service
  • Created PR 993 using ws/rewrite-core as base to fix issue 983 for visual-service

I didn't tried this time to update the type of executor/ExecuteScript to be compatible with the change done in this webdriverio commit because it was related to webdriver-image-comparison and since you switch to image-comparison-core, it's no longer a subject at this level.

Best regards,
Paul

@wswebcreation wswebcreation self-requested a review July 22, 2025 15:53
Copy link
Member

@wswebcreation wswebcreation left a comment

Choose a reason for hiding this comment

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

Hi @P-Courteille

Sorry for the late reply. Can you rebase and resolve the conflicts? A lot changed, also with the types. Sorry that this is taking more of your time. I'll wait releasing a new version when you're done with your changes

Not reading properly. Can you rebase all your PRs and point to main?

@wswebcreation
Copy link
Member

I belive this one can be closed so will close it in favor of the rest

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.

2 participants