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

Improve browser testing #269

Merged
merged 5 commits into from Mar 22, 2016

Conversation

Projects
None yet
2 participants
@evilaliv3
Contributor

evilaliv3 commented Mar 19, 2016

I've updated the selection of browser on which the libraries is tested.

index.html should be probably updated.

It is important nowadays to asses that the library is functiioning on IE11, Edge, and latest Android / Iphone that are all missing.

I would be a nice to have to start assessing also the code coverage of the tests exectuted on the library, and this would allow to spot deadbranches and outdated code.

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Mar 20, 2016

Collaborator

👍 for the missing browsers/nodejs versions.
Did you test the edge browser ? I can't start the test with edge as browser name but it works fine with microsoftedge.

Code coverage would be nice indeed. The results would be a bit off (some code paths are here only for some platforms) but that would be a start.

Collaborator

dduponchel commented Mar 20, 2016

👍 for the missing browsers/nodejs versions.
Did you test the edge browser ? I can't start the test with edge as browser name but it works fine with microsoftedge.

Code coverage would be nice indeed. The results would be a bit off (some code paths are here only for some platforms) but that would be a start.

@@ -1,4 +1,7 @@
JSZip
JSZip [![Build Status](https://api.travis-ci.org/Stuk/jszip.svg?branch=master)](http://travis-ci.org/Stuk/jszip) [![Code Climate](https://codeclimate.com/github/Stuk/jszip/badges/gpa.svg)](https://codeclimate.com/github/Stuk/jszip)

This comment has been minimized.

@dduponchel

dduponchel Mar 20, 2016

Collaborator

Putting the images here break the title. Any reason to move the build status ?
Also, I wouldn't put the codeclimate badge: the final score makes little sense as it currently includes generated files.

@dduponchel

dduponchel Mar 20, 2016

Collaborator

Putting the images here break the title. Any reason to move the build status ?
Also, I wouldn't put the codeclimate badge: the final score makes little sense as it currently includes generated files.

This comment has been minimized.

@evilaliv3
@evilaliv3
@evilaliv3

This comment has been minimized.

Show comment
Hide comment
@evilaliv3

evilaliv3 Mar 20, 2016

Contributor

you are right, probably we should write microsoftedge.

for what relates to putting the results on the top i like how it will come out like:
https://github.com/ghostbar/angular-zxcvbn

what do you think?

Contributor

evilaliv3 commented Mar 20, 2016

you are right, probably we should write microsoftedge.

for what relates to putting the results on the top i like how it will come out like:
https://github.com/ghostbar/angular-zxcvbn

what do you think?

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Mar 21, 2016

Collaborator

For the badges in the title, you need to put the line with ===== just below the title line with all the badges. Something like:

JSZip [badge1] [badge2] ...
=====

[saucelabs image]

I'm curious about codeclimate: why this one and not an other ?

Collaborator

dduponchel commented Mar 21, 2016

For the badges in the title, you need to put the line with ===== just below the title line with all the badges. Something like:

JSZip [badge1] [badge2] ...
=====

[saucelabs image]

I'm curious about codeclimate: why this one and not an other ?

@evilaliv3

This comment has been minimized.

Show comment
Hide comment
@evilaliv3

evilaliv3 Mar 21, 2016

Contributor

i'm experimenting with CodeClimate and Landscape.io

The first can provide you both the badges for their linter(that is anyhow unique in the suggestions and it's improving) and the coverage badge for the results you publish to them; The latter is more traditional and do only the linting, so you should eventually also setup Coveralls.io for that.

Any concern?

Contributor

evilaliv3 commented Mar 21, 2016

i'm experimenting with CodeClimate and Landscape.io

The first can provide you both the badges for their linter(that is anyhow unique in the suggestions and it's improving) and the coverage badge for the results you publish to them; The latter is more traditional and do only the linting, so you should eventually also setup Coveralls.io for that.

Any concern?

@evilaliv3

This comment has been minimized.

Show comment
Hide comment
@evilaliv3

evilaliv3 Mar 21, 2016

Contributor

Here you go :)

Contributor

evilaliv3 commented Mar 21, 2016

Here you go :)

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Mar 21, 2016

Collaborator

I never used these tools as a service and a quick search about code quality and github integration showed plenty of competitors, hence my question.
I'm ok with CodeClimate but you will need in that case to add the .codeclimate.yml file :-)

Collaborator

dduponchel commented Mar 21, 2016

I never used these tools as a service and a quick search about code quality and github integration showed plenty of competitors, hence my question.
I'm ok with CodeClimate but you will need in that case to add the .codeclimate.yml file :-)

Show outdated Hide outdated README.markdown
@evilaliv3

This comment has been minimized.

Show comment
Hide comment
@evilaliv3

evilaliv3 Mar 21, 2016

Contributor

we do not need any .codeclimate.yml; that is only if you would like to give specific directives.

Contributor

evilaliv3 commented Mar 21, 2016

we do not need any .codeclimate.yml; that is only if you would like to give specific directives.

@evilaliv3

This comment has been minimized.

Show comment
Hide comment
@evilaliv3

evilaliv3 Mar 21, 2016

Contributor

bam bam bam!

Contributor

evilaliv3 commented Mar 21, 2016

bam bam bam!

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Mar 22, 2016

Collaborator

But if we don't have any .codeclimate.yml to exclude dist/ files, we will keep a meaningless score. Either add this conf file to get a meaningful score or remove the CodeClimate badge.

Collaborator

dduponchel commented Mar 22, 2016

But if we don't have any .codeclimate.yml to exclude dist/ files, we will keep a meaningless score. Either add this conf file to get a meaningful score or remove the CodeClimate badge.

@evilaliv3

This comment has been minimized.

Show comment
Hide comment
@evilaliv3

evilaliv3 Mar 22, 2016

Contributor

bam bam bam bam!

Contributor

evilaliv3 commented Mar 22, 2016

bam bam bam bam!

dduponchel added a commit that referenced this pull request Mar 22, 2016

@dduponchel dduponchel merged commit d3e6dbd into Stuk:master Mar 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Mar 22, 2016

Collaborator

Thanks !

Collaborator

dduponchel commented Mar 22, 2016

Thanks !

@evilaliv3

This comment has been minimized.

Show comment
Hide comment
@evilaliv3

evilaliv3 Mar 22, 2016

Contributor

you are welcome @dduponchel

i just noticed that codeclimate do not accept partial configurations, so you would need to load this complete one or it will give you an error: evilaliv3@d8f4e5b

sorry for not noticing that before.

Contributor

evilaliv3 commented Mar 22, 2016

you are welcome @dduponchel

i just noticed that codeclimate do not accept partial configurations, so you would need to load this complete one or it will give you an error: evilaliv3@d8f4e5b

sorry for not noticing that before.

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Mar 22, 2016

Collaborator

Could you create an other pull request with the updated config file ?

Collaborator

dduponchel commented Mar 22, 2016

Could you create an other pull request with the updated config file ?

@evilaliv3

This comment has been minimized.

Show comment
Hide comment
@evilaliv3

evilaliv3 Mar 22, 2016

Contributor

here you go: #271

Contributor

evilaliv3 commented Mar 22, 2016

here you go: #271

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