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

Bump to a customised version of resemble for node 8+. #3

Merged
merged 1 commit into from
Mar 19, 2018
Merged

Bump to a customised version of resemble for node 8+. #3

merged 1 commit into from
Mar 19, 2018

Conversation

alexjeffburke
Copy link
Member

No description provided.

@alexjeffburke
Copy link
Member Author

This PR restores the ability to install the plugin on (at least) node 8.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 90.909% when pulling 96cb3f9 on alexjeffburke:master into 701e784 on unexpectedjs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 90.909% when pulling 96cb3f9 on alexjeffburke:master into 701e784 on unexpectedjs:master.

@coveralls
Copy link

coveralls commented Mar 19, 2018

Coverage Status

Coverage increased (+0.3%) to 90.909% when pulling 96cb3f9 on alexjeffburke:master into 701e784 on unexpectedjs:master.

@papandreou
Copy link
Member

Just as a sanity check I wanted to see the current version fail first, so I went rm -fr node_modules && npm install && npm test on the current master with node 8.9.3 -- and it worked:

> unexpected-resemble@3.1.0 test /home/andreas/work/unexpected-resemble
> mocha ./documentation/**/*.md ./test/**/*.js && npm run lint



  to-resemble
    ✓ example #1 (documentation/assertions/any/to-resemble.md:4:1) should fail with the correct error message (205ms)

  index
    ✓ example #1 (documentation/index.md:23:1) should fail with the correct error message (38ms)
    ✓ example #2 (documentation/index.md:45:1) should succeed (45ms)

  unexpected-resemble
    ✓ should succeed when the comparison is successful (58ms)
    ✓ should fail when the comparison is unsuccessful (137ms)
    ✓ should compare images provided as data: urls
    with mismatchPercentage in odd camel case
      ✓ should succeed when the comparison is successful (59ms)
      ✓ should fail when the comparison is unsuccessful (174ms)


  8 passing (746ms)

Could we take it from the top? What exactly did you see when it failed initially?

@alexjeffburke
Copy link
Member Author

Hmm we’re you on linux by chance? I’m pretty sure the issues start when you’re compiling with clang... I could reliably get it to fail until canvas v1.6.3 which has a fix in it specifically for that situation on macOS. Trying to think what else might be relevant - I was using node 8.9.1 so I think it was a thing. I noticed the travis job forces the use of gcc and am wondering if that masked it in CI.

@papandreou
Copy link
Member

Yeah, I'm on linux, I guess you're not?

@alexjeffburke
Copy link
Member Author

That’ll be it, it’s the compiler - my understanding of the the as-is version of nan is it’s unhappy with clang.

@alexjeffburke
Copy link
Member Author

And sorry, to make sure it’s captured - I was putting this together on macOS 10.12 :)

@papandreou papandreou merged commit 9742f98 into unexpectedjs:master Mar 19, 2018
@papandreou
Copy link
Member

Thanks, I've published 3.2.0 with this change. I bisected and found out that 7c54916 was what broke the browser build, so I also reverted that.

Tried getting rid of the resemble module and also tried upgrading resemblejs to the newest version. Both attempts caused the test suite to fail right and left, so...

Byebye

@alexjeffburke
Copy link
Member Author

Haha after my brief encounter I second the above :P Thanks for looking into it & getting the change out!

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.

None yet

3 participants