-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Switch to karma #8764
Switch to karma #8764
Conversation
I don't know what to do about the failing build on node 10. I can reproduce because I encounter another problem while installing. but some of our dependencies like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @wvanderp!
Could you please revert the stylistic and whitespace changes? Those make it more difficult to review and would clutter git history/git blame.
3e6783a
to
e3853c0
Compare
e3853c0
to
5305b4f
Compare
Sorry for the potential spam. I had to remove the formatting by hand and it didn't want to be removed. But it is gone now. I would advise to introduce some rules for committing the changes. |
@wvanderp big thanks for this contribution! |
TLDR: maintainability and flexibility Gains in maintainability: The main reason is that PhantomJS is depricated. Karma is a large ecosystem, and because of this, we gain a more stable future for our test runner and a lot of support online. After some tweaking of the config files, it is possible to debug karma test with the chrome dev tools. Gains in flexibility: Because of Karmas large ecosystem, we can get a lot of new functionality out of the box. There are plugins for most browsers under the sun. I currently only choose chrome because it is the most straightforward. But with some modifications (mainly removing es6 and adding polyfills), we can use IE11 or even PhantomJS as our runner. I have configured karma to collect test coverage. I think this is new for the project. Also a bit difficult to add to the old setup. |
See #8774 (comment), this is not really an issue for us anymore. 😉 |
Thanks for this very useful switch @wvanderp!
this was resolved in the meantime with #8766
I merged this more or less as is, such that work on #8774 (or #8811) isn't blocked anymore. I only removed the old "test runner" index.html for phantomjs for now, but there are quite a few remaining bits of codes referencing phantomjs around and, as you said, the documentation probably also needs to be updated. Let's track that in a separate ticket -> #8843. |
* Merge iD pull request #8764 into RapiD, replacing phantomjs with karma * Further fixes to get unit tests working. * Ditch the karma-remap-istanbul package, as it did not work in RapiD. Code coverage report seems unaffected. * Fix import assertion error now that we're using node 16. * Use import assertions syntax for importing JSON files Requires node 16.14 and up going forward Co-authored-by: Martin Raifer <martin@raifer.tech> Co-authored-by: Bryan Housel <bhousel@gmail.com>
* Merge iD pull request #8764 into RapiD, replacing phantomjs with karma * Further fixes to get unit tests working. * Ditch the karma-remap-istanbul package, as it did not work in RapiD. Code coverage report seems unaffected. * Fix import assertion error now that we're using node 16. * Use import assertions syntax for importing JSON files Requires node 16.14 and up going forward Co-authored-by: Martin Raifer <martin@raifer.tech> Co-authored-by: Bryan Housel <bhousel@gmail.com>
TLDR: I replaced phanton.js with karma and fixed the tests.
In this PR, I changed phatom.js for karma and fixed the newly failing tests.
What follows is a long list of changes:
There are also some remarks and issues:
sorry for all the white space changes it seems that these files were not touched for a long time.
I am happy to answer any questions or make any changes to this commit.