posthtml / posthtml-img-autosize

Auto setting the width and height of <img>
MIT License
28 stars 8 forks source link

Fix tests #13

Closed paleite closed 6 years ago

paleite commented 6 years ago

I've fixed the test that checks if an error was thrown, which revealed some bugs that I also fixed.

For completeness sake, I also added more tests.

maltsev commented 6 years ago

Thanks for your contribution!

Getting more tests is always good :-) Can you please fix the failed one, though?

paleite commented 6 years ago

Happy to help!

I noticed I had the wrong .eslintrc.json, which allowed me to use async/await locally, but failed when creating the PR. I've added a new commit and this passes the remote test, but there seems to be some issue with the dependency @babel/code-frame: https://travis-ci.org/posthtml/posthtml-img-autosize/builds/397194778

In other words. It works for Node 8 and upwards as expected, but not for Node 4 and Node 5.

How would you like to proceed?

maltsev commented 6 years ago

Thanks for fixing it!

It works for Node 8 and upwards as expected, but not for Node 4 and Node 5.

I think we shouldn't care about Node 4 and 5 anymore since their support has ended anyway. But we should care about Node 6, which is an LTS release and will be in a maintenance mode for another year.

Do you mind adding that version in .travis.yml, so we can be sure everything works well even with Node.js 6 before merging the PR into the master branch? It seems that I can't do it myself since a PR is allowed to be edited only by its author. Below are needed changes. Just save them in a file and run git apply that_file.

diff --git a/.travis.yml b/.travis.yml
index 1e41285..4692247 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,7 +1,6 @@
 sudo: false
 language: node_js
 node_js:
-  - stable
-  - "4"
-  - "5"
+  - "6"
   - "8"
+  - stable
paleite commented 6 years ago

Hi again,

I patched travis and committed it, but Travis timed out for some reason. I tried running it locally with node 6 and it passed all tests.

Is there a way to force a re-run? Judging by the output, it didn't even install the npm packages.

maltsev commented 6 years ago

I guess it was some temporary issue. I re-ran the tests and they passed successfully.

Thanks for fixing all these issues! I really appreciate it.

I'll release a new version later this evening.

paleite commented 6 years ago

No worries 😄

Glad I could help!

maltsev commented 6 years ago

I published it as version 0.1.3.