Skip to content
This repository was archived by the owner on Oct 1, 2022. It is now read-only.

Conversation

@MartijnR
Copy link
Member

@MartijnR MartijnR commented Jun 11, 2019

… tests

Not an actual pull request, but using as a tool to comment inline on the code.

Tests run about 6x faster with the new engine which is a nice improvement :)

I didn't comment on the tests you are already marked with TODO.


} );
// TODO do we need this?
// import { g } from '../docwin';
Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't think so.

} );
} );
// TODO I think I missed this initially. Need to revisit all tests.
// import helpers from '../helpers';
Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, though they are not currently used, I think axes are a major area for expansion of our spec, especially preceding-sibling I believe browser support them thankfully.

};

expect( test ).to.throw( g.win.XPathException.INVALID_EXPRESSION_ERR ); //,/DOM XPath Exception 51/);
// TODO Is this good enough?
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think so.


expect( test ).to.throw( g.win.XPathException.NAMESPACE_ERR );
// TODO Is this good enough?
expect( test ).to.throw();
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, think so.

expect( item ).to.not.equal( null );
expect( item.nodeType ).to.equal( 13 );
//TODO chrome/firefox do not support namespace:node() but we get
// same result with '.' - Is this node type important?
Copy link
Member Author

Choose a reason for hiding this comment

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

No, not important, I think. Can be commented out.

} );

it( 'returns empty result if the default namespace for the node is empty', () => {
xit( 'returns empty result if the default namespace for the node is empty', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think these 4 commented-out tests are important.



it( '+ works', () => {
xit( '+ works', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Important, all of the xit tests in this file

item = result.singleNodeValue;
expect( item ).to.not.equal( null );
expect( item.nodeType ).to.equal( 13 );
// TODO chrome/firefox do not support namespace::node but
Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't think this is important

it( 'combines different attributes on the same element', () => {
//TODO Is node order important? chrome vs firefox return different order.
xit( 'combines different attributes on the same element', () => {
helpers.checkNodeResult( "id('eee40')/attribute::*[2] | id('eee40')/attribute::*[1]", g.doc, [
Copy link
Member Author

Choose a reason for hiding this comment

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

That's fine. We can keep this commented out.


it( 'combines a namespace and attribute', () => {
//TODO Is node order important? chrome vs firefox return different order.
xit( 'combines a namespace and attribute', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

No, not important, I think.

@MartijnR
Copy link
Member Author

@vimemo, I made comments inline. Please let me know if I forgot anything or if you'd like to discuss further.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants