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

Support for adding path to alternative browser #61

Closed
1 of 4 tasks
SnoCold opened this issue Apr 4, 2021 · 18 comments · Fixed by #62
Closed
1 of 4 tasks

Support for adding path to alternative browser #61

SnoCold opened this issue Apr 4, 2021 · 18 comments · Fixed by #62
Assignees

Comments

@SnoCold
Copy link
Collaborator

SnoCold commented Apr 4, 2021

Summary

What: Ability to add Application Path for Custom Browser

I use Brave Browser (Chromium Based) for my testing purposes. It would be Great if I could add the path to my browser application in the headless browser object before building it

Why:

There are developers who prefer Brave, and it has lately been getting famous as an anti-Chrome Browser

Acceptance Criteria

Below is a list of tasks that must be completed before this issue can be closed.

  • Write documentation
  • Write unit tests
  • Write integration tests
  • {add more checkboxes required by this issue as needed}

Example Pseudo Code (for implementation)

// Add example pseudo code for implementation
@SnoCold
Copy link
Collaborator Author

SnoCold commented Apr 4, 2021

The code could be something like

const Sinco = new HeadlessBrowser();
Sinco.setChromeBinaryPath('/path/to/Binary')
await Sinco.build()
...//and then followed by usual code

@crookse
Copy link
Member

crookse commented Apr 4, 2021

this is a pretty good use case imo @SnoCold. @ebebbington, thoughts?

@SnoCold
Copy link
Collaborator Author

SnoCold commented Apr 4, 2021

Thank you so much @crookse . If accepted I would like if I could take this issue up. I would still need help in creating a general purpose test case for this custom path scenario.

@ebebbington
Copy link
Member

@crookse yeah I mean it sounds good!:) There definitely seems to be a usecase

@SnoCold if you want to take on this issue, you are more than welcome :) BUT it’s probably best you either:

  • branch off of the ‘add Firefox’ PR (because the API changes a bit, and adding new browsers will be easier I believe)
    Or
  • Wait until that Firefox PR is in master/main

Essentially, a new browser would have the same API as our chrome and Firefox one, ie copy the Firefox source code file to a new brave file, and just change it to interact with brave instead

@crookse
Copy link
Member

crookse commented Apr 4, 2021

@ebebbington, i think what @SnoCold is asking for is a way to make the chrome browser API dynamic and to have it use a different binary than the default one -- not that we support a different browser entirely through code, but we support different binaries that use the same browser engine.

this being said, maybe we change the APIs to reflect browser engine names and not browser names?

@SnoCold
Copy link
Collaborator Author

SnoCold commented Apr 4, 2021

Yes surely @ebebbington .

Yes @crookse , Exactly that way. I tested it in my local machine (on local Master). It utilizes the same chromedriver seamlessly.

@ebebbington
Copy link
Member

@ebebbington, i think what @SnoCold is asking for is a way to make the chrome browser API dynamic and to have it use a different binary than the default one -- not that we support a different browser entirely through code, but we support different binaries that use the same browser engine.

this being said, maybe we change the APIs to reflect browser engine names and not browser names?

Ah ok I see, my bad

yeah maybe browser engine names might be better - I think an extra property could be added in the ‘build’ method to make it easy to implement, it’s just whether the API (chrome dev tools api that is) would act the same

it seems like it should be as simple as:

  • renaming the files and references where applicable to “chromium”
  • Add a new property to the param in the “build” method (or add a param to it if one doesn’t already exist
  • hope everything just works :p

@SnoCold
Copy link
Collaborator Author

SnoCold commented Apr 4, 2021

I feel like I should 'await' the PR merge before commencing on this path 😅

@ebebbington
Copy link
Member

I feel like I should 'await' the PR merge before commencing on this path 😅

;) It’s close to being merged (just being reviewed at the moment) so we can let you know when it is :)

@ebebbington
Copy link
Member

@SnoCold it's merged into master now :) Feel free to work on this issue, if not (ie no time to do so) then let us know, and we can implement it

Any questions or whatever it may be, you'ree free to ask on the discord too: https://discord.gg/SgejNXq

I think this issue is also very similar to #33, which i reckon a PR for this issue, will close that issue

@SnoCold
Copy link
Collaborator Author

SnoCold commented Apr 10, 2021

Thanks a lot for informing me @ebebbington . I will get to it asap. 😄

@SnoCold
Copy link
Collaborator Author

SnoCold commented Apr 10, 2021

@crookse @ebebbington I have made the initial changes and tested them locally, they pass.
I have 2 topics I need help on.

  1. Should I rename (Firefox -> Gecko) and (Chrome -> Chromium) Globally, or do we assume the user will understand underlying engine and keep the names intact.
  2. I am not sure how do I write tests for custom browser path (as that is an end user preference path and not a universal thing). Maybe we can do that on docker but I still need your advice.

pushing to remote branch for saving current changes

SnoCold added a commit to SnoCold/sinco that referenced this issue Apr 10, 2021
@ebebbington
Copy link
Member

@crookse @ebebbington I have made the initial changes and tested them locally, they pass.
I have 2 topics I need help on.

  1. Should I rename (Firefox -> Gecko) and (Chrome -> Chromium) Globally, or do we assume the user will understand underlying engine and keep the names intact.
  2. I am not sure how do I write tests for custom browser path (as that is an end user preference path and not a universal thing). Maybe we can do that on docker but I still need your advice.

pushing to remote branch for saving current changes

  1. Yeah I would say chromium is better now now we can add support for a path to a chromium based browser. As for Gecko, I’m not too sure - isn’t gecko another driver? I also wonder if this might affect UX (people might not understand off the bat, that gecko = Firefox

  2. yeah it’s a tricky one, mainly because to test them locally, you need all that stuff installed yourself... I’d say definitely do it for the Docker tests - we might have to slightly modify our test files. I’m thinking we’d need to test if no path is provided (defaults to chrome I guess?), and if a path is provided (both chrome and brave), all without rewriting the same test code

I’d like to hear Eric’s view point too

@SnoCold
Copy link
Collaborator Author

SnoCold commented Apr 10, 2021

Yes the code modifications don't break existing tests, if no path is provided, the tests will take by default chrome/firefox default binary paths. The BuildOption for custom binary is optional.

@crookse
Copy link
Member

crookse commented Apr 10, 2021

  1. yea idk because i think when we mention gecko we'd have to be like "the gecko engine (e.g., used by firefox) ... " or something and that sounds kinda funky. let me see if i can write some documentation and have it make sense.

  2. we should have our tests be inclusive of both docker and local machine settings. i think that's the right answer. think about who might be using this tool. some don't even know how to use docker. again, we could document though. idk if that makes sense. i'll rephrase if it doesn't

@SnoCold
Copy link
Collaborator Author

SnoCold commented Apr 10, 2021

I see what you mean. I think leaving the name as-is to Firefox and Chrome is more intuitive. If someone has doubts, they can always refer the documentation.

As for the tests, I will start working on them. My laptop had to go to service today. It will take some time to be back, but I'll be sure to pick-up soon.

@SnoCold
Copy link
Collaborator Author

SnoCold commented Apr 12, 2021

I raised the PR, but no idea why the first test of the PR Actions failed. Also, please let me know if there is something more required in this PR or issue or anything that comes to mind. 😄

@ebebbington
Copy link
Member

Don’t worry about that failing workflow, it’s more an internal thing and doesn’t affect the status of your PR :) well review it when we can :)

SnoCold added a commit to SnoCold/sinco that referenced this issue Apr 13, 2021
SnoCold added a commit to SnoCold/sinco that referenced this issue Apr 13, 2021
ebebbington pushed a commit that referenced this issue Apr 14, 2021
* [#61] added BuildOption for custom browser binary

* [#61] added unit tests for the binaryPath feature

* [#61] did refactoring to reuse code

* [#61] deno fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants