Skip to content

Conversation

@Rora
Copy link
Contributor

@Rora Rora commented Apr 20, 2018

Resolves issue #1401

@asbjornu
Copy link
Member

asbjornu commented Feb 20, 2019

@Rora, thanks for this. Is it possible to test this in any way? If so, could you please add a test, rebase and resolve conflicts? 🙏 You can base your work on 1430d6b if you want, as I have resolved conflicts there.

@arturcic
Copy link
Member

arturcic commented Feb 20, 2019

As far as I remember, this is a nodejs app so the only way to test it is by running as nodejs app. Not sure we can include an unit test

@asbjornu
Copy link
Member

I don't mind having a test suite executed with npm test. If that's the only way to test this, then I would appreciate if we got something like that up and running.

@Rora
Copy link
Contributor Author

Rora commented Feb 20, 2019

I agree that it would be best to get a test suite up and running to be able to test this code. We should IMO log this as a separate issue as I don't have any experience in setting a nodejs test suite up and it would unnecessary prolong this PR.

@arturcic
Copy link
Member

+1 for a separate PR

@asbjornu
Copy link
Member

While I agree on doing a test suite as a separate PR, I would have preferred it the other way around; test suite first, features second. But seeing how long this PR has waited, I think it's fine merging this first.

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.

3 participants