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

Switch to karma #8764

Merged
merged 10 commits into from Dec 7, 2021
Merged

Switch to karma #8764

merged 10 commits into from Dec 7, 2021

Conversation

wvanderp
Copy link
Contributor

@wvanderp wvanderp commented Oct 24, 2021

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:

  • Added karma test runner
  • Removed phantom.js
  • Removed the polyfills for phantom.js
  • Replace the homegrown fake fetch for a module
  • Implemented code coverage and ignored them in git and npm
  • Fixed the issues caused by replacing the fake fetch
  • updated all test-related packages

There are also some remarks and issues:

  • some of the tests fail when the tests are run in a windowed environment. but not in a headless environment
  • This PR makes it difficult to run tests on IE11 because it does away with some of the polyfills.
    • On the other hand, you can select IE11 as a runner, so we can always add that back later.
  • This is a large change, and I am new to developing for ID, so this change should be tested both in development environments and CI to gain confidence in the change.
  • I didn't edit any of the documentation or code added for phantom.js

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.

@wvanderp
Copy link
Contributor Author

wvanderp commented Oct 24, 2021

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 @ideditor/country-coder, @ideditor/location-conflation, or osm-community-index don't support 10

Copy link
Member

@jfirebaugh jfirebaugh left a 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.

@wvanderp wvanderp force-pushed the switchToKarma branch 6 times, most recently from 3e6783a to e3853c0 Compare October 24, 2021 22:37
@wvanderp
Copy link
Contributor Author

wvanderp commented Oct 24, 2021

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.

@mbrzakovic
Copy link
Collaborator

@wvanderp big thanks for this contribution!
Could you please just shortly state what are the core gains for replacing phantom with karma?

@wvanderp
Copy link
Contributor Author

TLDR: maintainability and flexibility

Gains in maintainability:

The main reason is that PhantomJS is depricated.
See #7697

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.

@mbrzakovic mbrzakovic added this to the 2.21.0 milestone Oct 28, 2021
@mbrzakovic mbrzakovic mentioned this pull request Nov 16, 2021
7 tasks
@tyrasd tyrasd added the chore-build Improvements to the iD build scripts / CI environment label Dec 7, 2021
@tyrasd tyrasd mentioned this pull request Dec 7, 2021
@tyrasd
Copy link
Member

tyrasd commented Dec 7, 2021

This PR makes it difficult to run tests on IE11 because it does away with some of the polyfills.

See #8774 (comment), this is not really an issue for us anymore. 😉

@tyrasd tyrasd self-assigned this Dec 7, 2021
@tyrasd tyrasd merged commit 94db5c2 into openstreetmap:develop Dec 7, 2021
@tyrasd
Copy link
Member

tyrasd commented Dec 7, 2021

Thanks for this very useful switch @wvanderp!

I don't know what to do about the failing build on node 10

this was resolved in the meantime with #8766

I didn't edit any of the documentation or code added for phantom.js

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.

@wvanderp wvanderp deleted the switchToKarma branch January 25, 2022 11:38
Bonkles referenced this pull request in facebook/Rapid Mar 17, 2022
* 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>
Bonkles referenced this pull request in facebook/Rapid Mar 17, 2022
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore-build Improvements to the iD build scripts / CI environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants