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

Fix Jasmine reporter losing spec results when pretty printing JSDOM nodes #723

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@probablyup
Copy link
Contributor

probablyup commented Feb 19, 2016

JSDOM for some reason doesn't support attributeNode.nodeName. I looked at the HTML spec and it's safe to use attributeNode.name and attributeNode.value, so I adjusted the formatter to use what is available. I'll also send a patch to JSDOM to fix this on their side.

It should be here: https://github.com/tmpvar/jsdom/blob/fcece90f9f9505cfccbcdfac21fc66f40fcfae88/lib/jsdom/living/attributes/Attr-impl.js

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Feb 19, 2016

Thanks for the contribution! Would you mind adding a test for this? :)

@probablyup probablyup force-pushed the probablyup:fix-jasmine-formatter-jsdom-interface branch from e483de6 to 0ed6a2b Feb 19, 2016

@probablyup

This comment has been minimized.

Copy link
Contributor

probablyup commented Feb 19, 2016

@cpojer added!

@probablyup probablyup force-pushed the probablyup:fix-jasmine-formatter-jsdom-interface branch from 0ed6a2b to 9bb2a6f Feb 19, 2016

@probablyup

This comment has been minimized.

Copy link
Contributor

probablyup commented Feb 19, 2016

Note that the CI will fail until #716 is merged


var formatter;

describe('JasmineFormatter', function() {

This comment has been minimized.

@cpojer

cpojer Feb 19, 2016

Contributor

since we are here, would you mind using arrow functions and const and let all over this file? :) I'm trying to update all the tests to newer patterns.

@probablyup

This comment has been minimized.

Copy link
Contributor

probablyup commented Feb 19, 2016

Oh sure thing!


jest.autoMockOff();

var jasmine = require('../../../../vendor/jasmine/jasmine-1.3.0').jasmine;

This comment has been minimized.

@cpojer

cpojer Feb 19, 2016

Contributor

can you do something like:

const VENDOR_PATH = path.resolve(__dirname, '../../../../vendor');

and use that in your require calls? :)

it('should handle JSDOM nodes with Jasmine 1.x', function() {
formatter = new JasmineFormatter(jasmine);

expect(function() { formatter.prettyPrint(jsdom(fixture).body); }).not.toThrow();

This comment has been minimized.

@cpojer

cpojer Feb 19, 2016

Contributor

yeah, this should just be

expect(() => formatter.prettyPrint(jsdom(fixture).body)).not.toThrow();

make sure to keep within the 80 col limit! :)

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Feb 19, 2016

Awesome. Having a few hiccups internally but I'll merge this asap :)

Evan Jacobs
Fix Jasmine reporter losing spec results when pretty printing JSDOM n…
…odes

JSDOM for some reason doesn't support attributeNode.nodeName. I looked at the
HTML spec and it's safe to use attributeNode.name and attributeNode.value, so
I adjusted the formatter to use what is available. I'll also send a patch to
JSDOM to fix this on their side.

@probablyup probablyup force-pushed the probablyup:fix-jasmine-formatter-jsdom-interface branch from 9bb2a6f to fc3a33c Feb 19, 2016

@probablyup

This comment has been minimized.

Copy link
Contributor

probablyup commented Feb 19, 2016

@cpojer Pushed up an update, I think I hit all the things you mentioned

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Feb 19, 2016

lovely!

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Feb 19, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Feb 19, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Feb 19, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Feb 19, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@probablyup

This comment has been minimized.

Copy link
Contributor

probablyup commented Feb 19, 2016

🙌

@probablyup

This comment has been minimized.

Copy link
Contributor

probablyup commented Feb 19, 2016

Want me to rebase?

@cpojer

This comment has been minimized.

Copy link
Contributor

cpojer commented Feb 19, 2016

nope, just wait for the bot to close this for you.

@probablyup

This comment has been minimized.

Copy link
Contributor

probablyup commented Feb 19, 2016

zgi55

This issue was closed.

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