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.

Sign up for GitHub

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

Jump to bottom

Clean up OdtDocument cursor position tests #783

Merged
merged 5 commits into from
Sep 23, 2014
Merged

Clean up OdtDocument cursor position tests #783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c0a1acc
Cope with test names containing angle brackets
peitschie Aug 13, 2014
1cd6d61
Verify the number of steps vs. expected number of cursor positions
peitschie Aug 13, 2014
e72e8b8
Fix doc length check when iterating backwards
peitschie Aug 13, 2014
2185247
Report one test per cursor position document fragment
peitschie Aug 13, 2014
294542b
Print function result rather than function.toString for test description
peitschie Aug 21, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions webodf/lib/core/UnitTester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,8 +568,13 @@ core.UnitTester = function UnitTester() {
* @return {!string}
**/
function link(text, code) {
// NASTY HACK, DO NOT RE-USE. String concatenation with uncontrolled user input is a bad idea for building DOM
// fragments everyone. If you feel tempted to extract the HTML escape thing from here, please force yourself to
// visit http://shebang.brandonmintern.com/foolproof-html-escaping-in-javascript/ first, and learn a better
// approach to take.

return "<span style='color:blue;cursor:pointer' onclick='" + code + "'>"
+ text + "</span>";
+ text.replace(/</g, "&lt;") + "</span>";
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to instead add a generic encodeForHTML (or better named) method which encodes <, > and & and to use it in all places where text is added to the HTML output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hrm. Where else are you proposing this encode function being used?

I actually would like to never see such a thing in production code, as the somewhat uncontrolled string concatenation happening here seems like very bad practice to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm. Where else are you proposing this encode function being used?

Now that I tried to list them, there might be only that one other place in core.UnitTester which I saw initially, https://github.com/kogmbh/WebODF/blob/master/webodf/lib/core/UnitTester.js#L635 Somehow I expected more :)

This also made me look at BrowserRuntime.log where I learned that any log messages which start with < are set to innerHTML (thus need to be encoded) while all others are just passed into a new text node, so do not need encoding, okay. So still leaves the encoding problem with those inBrowser-special logging with custom spans, where the caller has to make sure things are properly encoded.
And I also learned from e.g. http://shebang.brandonmintern.com/foolproof-html-escaping-in-javascript/ that encoding is best left to the browser, e.g. by

function escapeHtml(str) {
    var div = document.createElement('div');
    div.appendChild(document.createTextNode(str));
    return div.innerHTML;
};

Hm. So what to do here without going over-board. Just the proposed fix still seems to be too hacky and done on case-by-case.

Oh well. Without a better idea, please just add a comment to it that this is an unclean fix to a certain problem.

And while talking, in line 637 it should be test.description(), not test.description, or? Please give that a patch as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brandon and I share the same opinion about the best way to do escape.

I don't mind leaving this as a nasty hack for the simple reason that if it looks like a bad hack, no-one will be tempted to copy it out of the file 🎌. Or... if they do, it should become extremely obvious to the reviewer that this code was never meant to make it into production!

But, I will happily comment the line to reinforce this!

Copy link
Contributor

Choose a reason for hiding this comment

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

Still slightly confused why only < is cared for and not also > (and perhaps & for a possible test name char)? Because browsers can do proper error resolving with those other entities (no < before -> show < as char, no entity name after & -> show as char)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For most browser display purposes, escaping the open-tag is sufficient to ensure the code isn't parsed as HTML. Escaping the ampersand isn't necessary with the test names I'm using so far, but it would make sense for future-proofing.

However, as discussed on the ML, I want to throw away all this code and replace it with jasmine anyway. As a result, I don't feel it's a good investment to do any real future-proofing on this part of the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mh, okay. I would have made the escaping catch here a bit more broad given that examples show some non-[azAZ] chars to expect, but if you think this will soon be replaced and until then should just be hacked on case-by-case, well, there are more important things to discuss about :)
So please go with this and rebase and merge 🐹

}
/**
* @type {function(!{description:string,suite:!Array.<string>,success:boolean,log:!Array.<{category:string,message:string}>,time:number})}
Expand Down Expand Up @@ -634,7 +639,7 @@ core.UnitTester = function UnitTester() {
+ link(testName, "runSuite(\"" + testName + "\");")
+ ": " + test.description() + "</span>");
} else {
runtime.log("Running " + testName + ": " + test.description);
runtime.log("Running " + testName + ": " + test.description());
}
tests = test.tests();
for (i = 0; i < tests.length; i += 1) {
Expand Down
168 changes: 87 additions & 81 deletions webodf/tests/ops/OdtDocumentTests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,11 +158,12 @@ ops.OdtDocumentTests = function OdtDocumentTests(runner) {
documentRoot;

serializer.filter = new OdfOrCursorNodeFilter();
runtime.log("Scenario: " + documentString);
t.segmentCount = segments.length;
t.expectedCursorPositionsCount = segments.length - 1; // e.g., <a>|</a> has two segments, but only one cursor position
r.shouldBe(t, "t.segmentCount > 1", "true");
documentRoot = createOdtDocument(segments.join(""));
t.documentLength = t.odtDocument.convertDomPointToCursorStep(documentRoot, documentRoot.childNodes.length, PREVIOUS);
r.shouldBe(t, "t.expectedCursorPositionsCount", "(t.documentLength+1)");

// Test iteration forward
for (position = 1; position < segments.length; position += 1) {
Expand All @@ -186,6 +187,7 @@ ops.OdtDocumentTests = function OdtDocumentTests(runner) {
// Test iteration backward
for (position = segments.length - 1; position > 0; position -= 1) {
setCursorPosition(step);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this removed here, but not in the loop "Test iteration forward"? Or rather, why is t.currentDocLength calculated and checked only in that loop, but not when going backwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that the length check was unnecessary the second time... but that logic is not re-assuring me still. I've reverted this part, and actually made it so the length is re-measured each time instead (i.e., fixing the check to be useful).

t.currentDocLength = t.odtDocument.convertDomPointToCursorStep(documentRoot, documentRoot.childNodes.length, PREVIOUS);
r.shouldBe(t, "t.currentDocLength", "t.documentLength");
t.expected = "<office:text>" +
segments.slice(0, position).join("") + "|" + segments.slice(position, segments.length).join("") +
Expand Down Expand Up @@ -363,76 +365,6 @@ ops.OdtDocumentTests = function OdtDocumentTests(runner) {
r.shouldBe(t, "t.rootToFocus", "1");
}

function testAvailablePositions_EmptyParagraph() {
// Examples from README_cursorpositions.txt
testCursorPositions("<text:p>|</text:p>");
// TODO behaviour is different from README_cursorpositions
// cursorPositionsTest("<text:p><text:span>|</text:span></text:p>");
// TODO behaviour is different from README_cursorpositions
// cursorPositionsTest("<text:p><text:span>|</text:span><text:span></text:span></text:p>");
// TODO behaviour is different from README_cursorpositions
// cursorPositionsTest("<text:p><text:span>|<text:span></text:span></text:span></text:p>");
testCursorPositions("<text:p>| </text:p>");
// TODO behaviour is different from README_cursorpositions
// cursorPositionsTest("<text:p> <text:span>| </text:span> </text:p>");
// TODO behaviour is different from README_cursorpositions
// cursorPositionsTest("<text:p> <text:span>| </text:span> <text:span> <text:span> </text:span> </text:span> </text:p>");
}
function testAvailablePositions_SimpleTextNodes() {
// Examples from README_cursorpositions.txt
testCursorPositions("<text:p>|A|B|C|</text:p>");
}
function testAvailablePositions_MixedSpans() {
// Examples from README_cursorpositions.txt
testCursorPositions("<text:p><text:span>|A|B|</text:span>C|</text:p>");
testCursorPositions("<text:p>|A|<text:span>B|</text:span>C|</text:p>");
testCursorPositions("<text:p>|A|<text:span>B|C|</text:span></text:p>");
testCursorPositions("<text:p><text:span>|A|<text:span>B|</text:span></text:span>C|</text:p>");
testCursorPositions("<text:p>|A|<text:span><text:span>B|</text:span>C|</text:span></text:p>");
}
function testAvailablePositions_Whitespace() {
// Examples from README_cursorpositions.txt
testCursorPositions("<text:p>|A| |B|C|</text:p>");
testCursorPositions("<text:p> |A| </text:p>");
testCursorPositions("<text:p>|A| |B|</text:p>");
testCursorPositions("<text:p> |A| | B| </text:p>");
testCursorPositions("<text:p> <text:span> |a| </text:span> </text:p>");
testCursorPositions("<text:p> <text:span>|a| | </text:span> <text:span> b|</text:span> </text:p>");
// TODO behaviour is different from README_cursorpositions
// cursorPositionsTest("<text:p> <text:span> |a<text:span>|</text:span> </text:span> </text:p>");
testCursorPositions("<text:p> <text:span> |a|<text:span></text:span> </text:span> </text:p>");
testCursorPositions("<text:p><text:span></text:span> |a|</text:p>");
}
function testAvailablePositions_SpaceElements() {
// Examples from README_cursorpositions.txt
testCursorPositions("<text:p>|A|<text:s> </text:s>|B|C|</text:p>");
// Unexpanded spaces - Not really supported, but interesting to test
testCursorPositions("<text:p>|A|<text:s></text:s>|B|C|</text:p>");
// Slight span nesting and positioning
testCursorPositions("<text:p><text:span>|<text:s> </text:s>|</text:span></text:p>");
// TODO behaviour is different from README_cursorpositions
// cursorPositionsTest("<text:p> <text:span>|A| |</text:span> <text:s></text:s>| <text:span><text:s> </text:s>|B|</text:span> </text:p>");
testCursorPositions("<text:p>|<text:tab> </text:tab>|<text:s> </text:s>|<text:s> </text:s>|</text:p>");
testCursorPositions("<text:p>|<text:tab> </text:tab>| |<text:s> </text:s>|</text:p>");
testCursorPositions("<text:p>|a| | <text:s> </text:s>| </text:p>");
testCursorPositions("<text:p><e:editinfo></e:editinfo><text:span></text:span>|a|<text:s> </text:s>|<text:s> </text:s>|<text:span></text:span><text:span></text:span></text:p>");
}
function testAvailablePositions_DrawElements() {
testCursorPositions("<text:p>|<draw:frame text:anchor-type=\"as-char\"><draw:image><office:binary-data>data</office:binary-data></draw:image></draw:frame>|</text:p>");
testCursorPositions("<text:p><text:span>|<draw:frame text:anchor-type=\"as-char\"><draw:image><office:binary-data>data</office:binary-data></draw:image></draw:frame>|</text:span></text:p>");
}
function testAvailablePositions_Annotations() {
testCursorPositions('<text:p>|a|b|<office:annotation><text:list><text:list-item><text:p>|</text:p></text:list-item></text:list></office:annotation>|c|d|<office:annotation-end></office:annotation-end>1|2|</text:p>');
testCursorPositions('<text:p>|a|<office:annotation><text:list><text:list-item><text:p>|b|</text:p></text:list-item></text:list></office:annotation>|c|<office:annotation-end></office:annotation-end>1|2|</text:p>');
testCursorPositions('<text:p>|a|<office:annotation><text:list><text:list-item><text:p>|b|</text:p></text:list-item></text:list></office:annotation>|<office:annotation-end></office:annotation-end>1|2|</text:p>');
}
function testAvailablePositions_BetweenAnnotationAndSpan() {
testCursorPositions('<text:p>|a|b|<office:annotation><text:list><text:list-item><text:p>|</text:p></text:list-item></text:list></office:annotation><text:span>|c|d|</text:span><office:annotation-end></office:annotation-end>1|2|</text:p>');
testCursorPositions('<text:p>|a|<html:div class="annotationWrapper"><office:annotation><text:list><text:list-item><text:p>|b|</text:p></text:list-item></text:list></office:annotation></html:div><html:span class="webodf-annotationHighlight">|c|</html:span><office:annotation-end></office:annotation-end>1|2|</text:p>');
testCursorPositions('<text:p>|a|<html:div class="annotationWrapper"><office:annotation><text:list><text:list-item><text:p>|b|</text:p></text:list-item></text:list></office:annotation></html:div><html:span class="webodf-annotationHighlight">|</html:span><office:annotation-end></office:annotation-end>1|2|</text:p>');
}


function getTextNodeAtStep_BeginningOfTextNode() {
var doc = createOdtDocument("<text:p>ABCD</text:p>");
t.paragraph = doc.getElementsByTagNameNS(odf.Namespaces.textns, "p")[0];
Expand Down Expand Up @@ -624,6 +556,89 @@ ops.OdtDocumentTests = function OdtDocumentTests(runner) {
r.shouldBe(t, "t.domPosition.textNode.previousSibling.localName", "'cursor'");
}

function cursorPositionTests() {
// Examples from README_cursorpositions.txt

return [
// *************** Empty Paragraph *************** //
// Examples from README_cursorpositions.txt
"<text:p>|</text:p>",
// TODO behaviour is different from README_cursorpositions
// "<text:p><text:span>|</text:span></text:p>",
// TODO behaviour is different from README_cursorpositions
// "<text:p><text:span>|</text:span><text:span></text:span></text:p>",
// TODO behaviour is different from README_cursorpositions
// "<text:p><text:span>|<text:span></text:span></text:span></text:p>",
"<text:p>| </text:p>",
// TODO behaviour is different from README_cursorpositions
// "<text:p> <text:span>| </text:span> </text:p>",
// TODO behaviour is different from README_cursorpositions
// "<text:p> <text:span>| </text:span> <text:span> <text:span> </text:span> </text:span> </text:p>",


// *************** Simple Text Nodes *************** //
"<text:p>|A|B|C|</text:p>",


// *************** Mixed spans *************** //
"<text:p><text:span>|A|B|</text:span>C|</text:p>",
"<text:p>|A|<text:span>B|</text:span>C|</text:p>",
"<text:p>|A|<text:span>B|C|</text:span></text:p>",
"<text:p><text:span>|A|<text:span>B|</text:span></text:span>C|</text:p>",
"<text:p>|A|<text:span><text:span>B|</text:span>C|</text:span></text:p>",


// *************** Whitespace *************** //
"<text:p>|A| |B|C|</text:p>",
"<text:p> |A| </text:p>",
"<text:p>|A| |B|</text:p>",
"<text:p> |A| | B| </text:p>",
"<text:p> <text:span> |a| </text:span> </text:p>",
"<text:p> <text:span>|a| | </text:span> <text:span> b|</text:span> </text:p>",
// TODO behaviour is different from README_cursorpositions
// cursorPositionsTest("<text:p> <text:span> |a<text:span>|</text:span> </text:span> </text:p>",
"<text:p> <text:span> |a|<text:span></text:span> </text:span> </text:p>",
"<text:p><text:span></text:span> |a|</text:p>",


// *************** Space elements *************** //
"<text:p>|A|<text:s> </text:s>|B|C|</text:p>",
// Unexpanded spaces - Not really supported, but interesting to test
"<text:p>|A|<text:s></text:s>|B|C|</text:p>",
// Slight span nesting and positioning
"<text:p><text:span>|<text:s> </text:s>|</text:span></text:p>",
// TODO behaviour is different from README_cursorpositions
// cursorPositionsTest("<text:p> <text:span>|A| |</text:span> <text:s></text:s>| <text:span><text:s> </text:s>|B|</text:span> </text:p>",
"<text:p>|<text:tab> </text:tab>|<text:s> </text:s>|<text:s> </text:s>|</text:p>",
"<text:p>|<text:tab> </text:tab>| |<text:s> </text:s>|</text:p>",
"<text:p>|a| | <text:s> </text:s>| </text:p>",
"<text:p><e:editinfo></e:editinfo><text:span></text:span>|a|<text:s> </text:s>|<text:s> </text:s>|<text:span></text:span><text:span></text:span></text:p>",


// *************** Draw elements *************** //
"<text:p>|<draw:frame text:anchor-type=\"as-char\"><draw:image><office:binary-data>data</office:binary-data></draw:image></draw:frame>|</text:p>",
"<text:p><text:span>|<draw:frame text:anchor-type=\"as-char\"><draw:image><office:binary-data>data</office:binary-data></draw:image></draw:frame>|</text:span></text:p>",


// *************** Annotations *************** //
'<text:p>|a|b|<office:annotation><text:list><text:list-item><text:p>|</text:p></text:list-item></text:list></office:annotation>|c|d|<office:annotation-end></office:annotation-end>1|2|</text:p>',
'<text:p>|a|<office:annotation><text:list><text:list-item><text:p>|b|</text:p></text:list-item></text:list></office:annotation>|c|<office:annotation-end></office:annotation-end>1|2|</text:p>',
'<text:p>|a|<office:annotation><text:list><text:list-item><text:p>|b|</text:p></text:list-item></text:list></office:annotation>|<office:annotation-end></office:annotation-end>1|2|</text:p>',


// *************** Annotations with highlighting *************** //
'<text:p>|a|b|<office:annotation><text:list><text:list-item><text:p>|</text:p></text:list-item></text:list></office:annotation><text:span>|c|d|</text:span><office:annotation-end></office:annotation-end>1|2|</text:p>',
'<text:p>|a|<html:div class="annotationWrapper"><office:annotation><text:list><text:list-item><text:p>|b|</text:p></text:list-item></text:list></office:annotation></html:div><html:span class="webodf-annotationHighlight">|c|</html:span><office:annotation-end></office:annotation-end>1|2|</text:p>',
'<text:p>|a|<html:div class="annotationWrapper"><office:annotation><text:list><text:list-item><text:p>|b|</text:p></text:list-item></text:list></office:annotation></html:div><html:span class="webodf-annotationHighlight">|</html:span><office:annotation-end></office:annotation-end>1|2|</text:p>'

].map(function(testInput) {
return {
name: "cursorPositions " + testInput,
f: testCursorPositions.bind(undefined, testInput)
};
});
}

this.setUp = function () {
var doc, stylesElement;
testarea = core.UnitTest.provideTestAreaDiv();
Expand Down Expand Up @@ -656,15 +671,6 @@ ops.OdtDocumentTests = function OdtDocumentTests(runner) {
testFixCursorPositions_CursorNearParagraphStart_ForwardSelection,
testFixCursorPositions_CursorNearParagraphStart_ReverseSelection,

testAvailablePositions_EmptyParagraph,
testAvailablePositions_SimpleTextNodes,
testAvailablePositions_MixedSpans,
testAvailablePositions_Whitespace,
testAvailablePositions_SpaceElements,
testAvailablePositions_DrawElements,
testAvailablePositions_Annotations,
testAvailablePositions_BetweenAnnotationAndSpan,

getTextNodeAtStep_BeginningOfTextNode,
getTextNodeAtStep_EndOfTextNode,
getTextNodeAtStep_PositionWithNoTextNode_AddsNewNode_Front,
Expand All @@ -680,7 +686,7 @@ ops.OdtDocumentTests = function OdtDocumentTests(runner) {
getTextNodeAtStep_At4_PutsTargetMemberCursor_BeforeTextNode,
getTextNodeAtStep_AfterNonText_PutsTargetMemberCursor_BeforeTextNode,
getTextNodeAtStep_EmptyP_MovesAllCursors_BeforeTextNode
]);
]).concat(cursorPositionTests());
};
this.asyncTests = function () {
return [
Expand Down
4 changes: 2 additions & 2 deletions webodf/tests/tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ function runSuite(name) {
}
function runTest(suite, name) {
"use strict";
runtime.getWindow().location.search = "?suite=" + suite + "&test=" + name;
runtime.getWindow().location.search = "?suite=" + suite + "&test=" + encodeURIComponent(name);
}

function runSelectedTests(selectedTests) {
Expand All @@ -211,7 +211,7 @@ function getTestNameFromUrl(selectedTests) {
return;
}
if (options.test) {
selectedTests.testNames = options.test.split(",");
selectedTests.testNames = options.test.split(",").map(decodeURIComponent);
}
}

Expand Down