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

fix: add Chrome/ium fallback for getBrowserInfo() #19

Merged
merged 3 commits into from
Jun 3, 2021
Merged

fix: add Chrome/ium fallback for getBrowserInfo() #19

merged 3 commits into from
Jun 3, 2021

Conversation

rugk
Copy link
Member

@rugk rugk commented Jun 1, 2021

Note that:

  • Yes, these added tests fail. You can anyway only manually run them for now, but they are useful enough in this way to test if . (The old ones are broken anyway, if someone cares to fix them, feel free to do so. 🤔 )
    See https://github.com/TinyWebEx/common/blob/master/CONTRIBUTING.md#tests on how to run these.
  • Also, the tests are made to always assume true. If you know any way to exclude tests to run on certain browsers with mocha, feel free to change/suggest it to me. Otherwise, as said, let*'s just keep them as such.
    It's just easier to test this way (did so in Chrome/ium) than to keep watching for console messages or even CSS classes.
  • Also I had to export all these functions, but really… they are actually useful if one wants to use them as they are.

Also I just noticed it would be useful to move that JS file to https://github.com/TinyWebEx/EnvironmentDetector (maybe name it SystemDetector or so) and include it as a dependency here, as that is much more useful.

IIRC EnvironmentDetector worked roughly, but I was never quite convinced of it and thus did not use it yet. But sure we can make use of the simple functions here if we move them there.

Fixes #18

@rugk
Copy link
Member Author

rugk commented Jun 1, 2021

Pls review @tdulcet. (I cannot assign you here as you are not a part of the official maintainers here.)

Copy link

@tdulcet tdulcet left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for fixing this!

  • Yes, these added tests fail. You can anyway only manually run them for now, but they are useful enough in this way to test if .

Well, we could always setup CI to run them automatically in Firefox and Chrome, although that would probably be for a separate PR. Having tests that purposefully fail would also make the results more difficult to interpret.

@rugk
Copy link
Member Author

rugk commented Jun 2, 2021

Well, we could always setup CI to run them automatically in Firefox and Chrome, although that would probably be for a separate PR.

Oh yes, and for these tests you'd need to dynamically load the add-on into the browser etc.
Maybe it's possible with some libs and so on, but in the past it either was not possible or I did not tackle it, so well… this is unfortunately the state we have.

I certainly agree a CI would be much more than awesome. It's just (probably) a lot of work.

@rugk
Copy link
Member Author

rugk commented Jun 2, 2021

Okay, as I don't like untracked technical depth, I've created an issue here to track this and described an approach in order to get started: TinyWebEx/common#16

Tagged it with "help wanted", so if anyone wants to help there, that would be gladly appreciated.

@tdulcet
Copy link

tdulcet commented Jun 3, 2021

Oh yes, and for these tests you'd need to dynamically load the add-on into the browser etc.
Maybe it's possible with some libs and so on, but in the past it either was not possible or I did not tackle it, so well… this is unfortunately the state we have.

You can always use Mozilla's web-ext CLI tool, at least for Firefox. Although, it would probably be better to use something like Karma or Selenium/WebDriver to open the browser and run the tests.

I certainly agree a CI would be much more than awesome. It's just (probably) a lot of work.

I have never done extension testing, but generally cross browser testing is not too difficult (see lathonez/clicker#291 for example). Writing the tests is the hard part… 😉

@rugk
Copy link
Member Author

rugk commented Jun 3, 2021

Sure… but let's discuss that in the other issue, where it's intended to be… 😉

@rugk rugk merged commit b3a4ff5 into master Jun 3, 2021
@rugk rugk deleted the testEnv branch June 3, 2021 17:14
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.

getBrowserInfo() errors in Chrome/Chromium
2 participants