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

Add Karma browser testing for the API tests #461

Merged
merged 1 commit into from Mar 6, 2019

Conversation

holgerd77
Copy link
Member

This PR adds browser API testing to the VM.

It uses a Karma setup similar to the one in the tx library, adds a new npm script command testAPI:browser and integrates into CI.

This should help prevent issues like #457. I tested this specific case by injecting the error in a matching test and this would actually break the test run.

There were many files not running through and after some experimenting I decided not to try to fix since this seemed to be too much which would have to be addressed. Instead this PR provides the ground for further fixes by providing an initial list of running + excluded test files in the Karma configuration (karma.conf.js). File runs can then fixed case-by-case by opening separate PRs.

One exception to the non-fix approach is that I replaced promisify in the test files with a fitting polyfill/shim where it fixed the tests.

@coveralls
Copy link

coveralls commented Mar 4, 2019

Coverage Status

Coverage remained the same at 94.657% when pulling 67d5d0b on add-api-browser-testing into 0162d47 on master.

@holgerd77
Copy link
Member Author

Had to fix some linting errors and do some CI config modification, but this is now running through and should be ready for review.

Copy link
Contributor

@s1na s1na 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, thanks. I'm now on mobile however, will try to run locally when I'm behind the laptop.

@holgerd77
Copy link
Member Author

Rebased this.

@holgerd77
Copy link
Member Author

@s1na If you could do a short laptop check this would be appreciated, I think it would be nice if we merge early here then this could be already integrated e.g. along #462.

@holgerd77
Copy link
Member Author

Hi @s1na, since this is not touching any production code and improvements can easily be done on top, I would admin-merge this now, then Jacky can do subsequent test work on top and directly consider this along with his new tests.

I will also set the browser tests to being "required" for CI to pass from now on.

@holgerd77 holgerd77 merged commit 9eb779f into master Mar 6, 2019
@holgerd77 holgerd77 deleted the add-api-browser-testing branch March 6, 2019 09:59
@s1na
Copy link
Contributor

s1na commented Mar 6, 2019

Makes sense. Sorry for not testing in time...

@holgerd77
Copy link
Member Author

No problem. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants