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

Feature/karma unit test #8

Merged
merged 12 commits into from
Nov 15, 2018
Merged

Feature/karma unit test #8

merged 12 commits into from
Nov 15, 2018

Conversation

GianlucaGuarini
Copy link
Contributor

I did my best here to test the Vue files (that it's ridiculously complex). Unfortunately it's possible only via Karma.js that is well known to be buggy and overcomplicate. The coverage can't be generated properly as well but at least we have something

Copy link
Contributor

@nirazul nirazul 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. I have no idea what happens in the karma config, so it's hard to know if there are errors or problems. Apart from that, I could understand everything.

command: |
npm run cov
npm run cov-html
# - run:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment as to why it was disabled? It's hard to understand just from looking at it, what the intention was. Questions: Why was it disabled? When will it be ready to be reenabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a link to the coverage issue I had gotwarlost/istanbul#602. We have now 2 possibilities ahead of us:

  • Wait that the issue will be solved directly in the istanbul core
  • Trying to find out why this issue appears in in vue-ui. In this case I have spent already too much time on it and I had to opt in for a pragmatic solution in the short term

Copy link
Contributor

@nirazul nirazul Nov 6, 2018

Choose a reason for hiding this comment

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

@GianlucaGuarini
That's fine. It's just a suggestion for future issues like these :)

I just wanted to point out that it's important that such information is delivered via comment. Else it's forgotten and lost in the flow.
An additional comment would be even better. Was it commented out to solve the issue? Is it a workaround or a fix? Is it temporary or will ist stay that way for the time being?

@nirazul nirazul assigned faebeee and unassigned nirazul Nov 5, 2018
@coveralls
Copy link

Pull Request Test Coverage Report for Build 69

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+28.1%) to 63.043%

Totals Coverage Status
Change from base Build 58: 28.1%
Covered Lines: 91
Relevant Lines: 126

💛 - Coveralls

@GianlucaGuarini GianlucaGuarini merged commit d0ee3c7 into develop Nov 15, 2018
@GianlucaGuarini GianlucaGuarini deleted the feature/karma-unit-test branch November 15, 2018 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants