Harden test-cases and align with other implementations #84

ZauberNerd · 2017-02-12T21:19:27Z

This commit refactors and adds more tests to the parse() implementation
to align with similar libraries's behaviour, e.g.:

When running the new test suite against the old lib/parse.js implementation
a few tests were failing:

31 passing (177ms)
10 failing

1) parse() real should throw an Error when parsing an empty real:
    AssertionError: Missing expected exception..

2) parse() string should parse a self closing string:
    AssertionError: null === ''

3) parse() string should parse an empty string:
    AssertionError: null === ''

4) parse() string should parse a string with comments:
    AssertionError: 'a comment  string' === 'a string'

5) parse() array should parse empty elements inside an array:
    AssertionError: [ false ] deepEqual [ '', false ]

6) parse() dict should throw if key is missing:
    AssertionError: Missing expected exception..

7) parse() dict should throw if two keys follow each other:
    AssertionError: Missing expected exception..

8) parse() dict should throw if value is missing:
    AssertionError: Missing expected exception..

9) parse() dict should parse an empty key:
    AssertionError: {} deepEqual { '': '1' }

10) parse() dict should parse an empty value:
    AssertionError: { a: null } deepEqual { a: '' }

When executing the new implementation of lib/parse.js agains the old
tests the following tests were failing:

25 passing (143ms)
3 failing

1) plist parse() should parse an empty <key/> in a dictionary:
    AssertionError: false == true

2) plist parse() should parse an empty <key></key> in a dictionary:
    AssertionError: false == true

3) plist parse() should parse an empty <key></key> and <string></string> in dictionary with more data:
    AssertionError: { '': '', UIRequiredDeviceCapabilities: [ 'armv7' ] } deepEqual { UIRequiredDeviceCapabilities: [ 'armv7' ] }

As the output above shows, most of the issue are with the handling of
empty strings and empty dictionary keys.
Added with the new implementation are custom Errors for invalid
input, better handling of comments inside tags and a more aligned
handling of empty keys/strings.

I know this is a huge change, so I'd understand if you're hesitant to merge this,
but let me know your thoughts.
Initially I only wanted to fix the handling of empty strings/keys but then I got a bit carried away.

Related issues: #79 #71 #66 #81 #80 #67

This commit refactors and adds more tests to the `parse()` implementation to align with similar libraries's behaviour, e.g.: - https://github.com/Microsoft/node-fast-plist - https://docs.python.org/3/library/plistlib.html With the new tests a few of the tests were failing: ``` 31 passing (177ms) 10 failing 1) parse() real should throw an Error when parsing an empty real: AssertionError: Missing expected exception.. 2) parse() string should parse a self closing string: AssertionError: null === '' 3) parse() string should parse an empty string: AssertionError: null === '' 4) parse() string should parse a string with comments: AssertionError: 'a comment string' === 'a string' 5) parse() array should parse empty elements inside an array: AssertionError: [ false ] deepEqual [ '', false ] 6) parse() dict should throw if key is missing: AssertionError: Missing expected exception.. 7) parse() dict should throw if two keys follow each other: AssertionError: Missing expected exception.. 8) parse() dict should throw if value is missing: AssertionError: Missing expected exception.. 9) parse() dict should parse an empty key: AssertionError: {} deepEqual { '': '1' } 10) parse() dict should parse an empty value: AssertionError: { a: null } deepEqual { a: '' } ``` This commit also refactors the `lib/parse.js` to pass these tests. When executing the new implementation of `lib/parse.js` agains the old tests the following tests were failing: ``` 25 passing (143ms) 3 failing 1) plist parse() should parse an empty <key/> in a dictionary: AssertionError: false == true 2) plist parse() should parse an empty <key></key> in a dictionary: AssertionError: false == true 3) plist parse() should parse an empty <key></key> and <string></string> in dictionary with more data: AssertionError: { '': '', UIRequiredDeviceCapabilities: [ 'armv7' ] } deepEqual { UIRequiredDeviceCapabilities: [ 'armv7' ] } ``` As the output above shows, most of the issue are with the handling of empty strings and empty dictionary keys. Added with the new implementation are custom Errors for unexpected input, better handling of comments inside tags and a more aligned handling of empty keys/strings.

ZauberNerd · 2017-02-12T21:23:25Z

The browser tests are failing because of: https://docs.travis-ci.com/user/pull-requests#Pull-Requests-and-Security-Restrictions

ZauberNerd · 2017-02-19T11:29:48Z

@TooTallNate @mreinstein ping, any update on this?

mreinstein

@ZauberNerd overall, good changes. I'm sorry it took so long to get around to this. most of my comments are fairly trivial.

Another issue is I'd like to ensure the tests pass.

mreinstein · 2017-02-19T18:08:56Z

lib/parse.js

-      // ignore comment nodes (text)
-      if (!shouldIgnoreNode(node.childNodes[i])) {
-        new_arr.push( parsePlistXML(node.childNodes[i]));
+    if (!isEmptyNode(node)) {


I'd prefer to return early rather than wrap the whole thing in an if block. results in less nesting. e.g.,

if(isEmptyNode(node)) { return []; }

It also eliminates the "not" modifier (!) which makes the if condition ever-so-slightly easier to read.

mreinstein · 2017-02-19T18:09:55Z

lib/parse.js

-      if (!shouldIgnoreNode(node.childNodes[i])) {
-        if (key === null) {
+    counter = 0;
+    if (!isEmptyNode(node)) {


same here in terms of early return.

mreinstein · 2017-02-19T18:11:39Z

lib/parse.js

-      if (!shouldIgnoreNode(node.childNodes[i])) {
-        res = parsePlistXML(node.childNodes[i]);
-        if (null != res) new_arr.push(res);
+    if (!isEmptyNode(node)) {


another early return opportunity

mreinstein · 2017-02-19T18:12:29Z

lib/parse.js

    res = '';
-    if(node.childNodes[0]) {
+    if(!isEmptyNode(node)) {
      res = node.childNodes[0].nodeValue;


another early return opportunity. I think you can just return node.childNodes[0].nodeValue rather than filling a variable and returning that?

mreinstein · 2017-02-19T18:12:58Z

lib/parse.js

+    if(!isEmptyNode(node)) {
+      for (i=0; i < node.childNodes.length; i++) {
+        var type = node.childNodes[i].nodeType;
+        if (type === 3 || type === 4) {


rather than using these magic numbers 3 and 4 can you define them as node type constants?

mreinstein · 2017-02-19T18:13:47Z

lib/parse.js

-      if (node.childNodes[d].nodeType === 3) {
-        res += node.childNodes[d].nodeValue;
+    for (i=0; i < node.childNodes.length; i++) {
+      if (node.childNodes[i].nodeType === 3) {


I know you didn't write this code; it pre-dates your additions. But while you're in here, can you replace the magic number usage here too?

mreinstein · 2017-02-19T18:14:25Z

lib/parse.js

-    for (d=0; d < node.childNodes.length; d++) {
-      if (node.childNodes[d].nodeType === 3) {
-        res += node.childNodes[d].nodeValue.replace(/\s+/g, '');
+    if (!isEmptyNode(node)) {


another early return opportunity I think

mreinstein

changes look good. I'll merge after this change

mreinstein · 2017-02-19T20:55:38Z

lib/parse.js

@@ -10,6 +10,12 @@ var DOMParser = require('xmldom').DOMParser;

 exports.parse = parse;

+var NODE_TYPES = {


let's do these instead please:

TEXT_NODE = 3 CDATA_NODE = 4 COMMENT_NODE = 8

Ok, will do.
Btw: https://docs.travis-ci.com/user/sauce-connect/ and https://docs.travis-ci.com/user/jwt might help with getting the sauce labs tests to run for PRs from forks.

I don't think I have access to sauce, nor am I familiar with the service in great detail. @TooTallNate care to take this on?

Otherwise we could just disable them for PRs - I think they'll still be run on master merge, see this section: https://docs.travis-ci.com/user/pull-requests#Pull-Requests-and-Security-Restrictions

yeah but the problem with that is it's more work to verify PRs are good before they're merged. I would rather not merge anything to master and have the tests fail after the fact.

mreinstein · 2017-02-19T21:02:33Z

@ZauberNerd can you open a separate issue for the sauce labs change please?

ZauberNerd · 2017-04-11T11:37:53Z

@mreinstein do you have an idea when you are going to release a new version to npm with these changes included?

The `plist` module had a bug when parsing an empty key which has since been fixed and released in a new version. This commit updates the `plist` dependency to the latest version in order to remove a fix we had to introduce in this project to work around that issue. Ref: TooTallNate/plist.js#84

…TooTallNate/plist.js#84. Resolves #151

mreinstein · 2022-04-26T18:54:29Z

@ZauberNerd sorry for the crazy long reply delay. I'm hoping to lump together all of these changes that are considered "API breaking" in the next version (v4)

ZauberNerd changed the title ~~Harden test-cases and implementation to align with other implementations~~ Harden test-cases and align with other implementations Feb 12, 2017

mreinstein requested changes Feb 19, 2017

View reviewed changes

ZauberNerd force-pushed the harden-parser branch from 40aae71 to 792b920 Compare February 19, 2017 20:52

mreinstein reviewed Feb 19, 2017

View reviewed changes

review feedback: early returns and constants for nodeTypes

7619537

ZauberNerd force-pushed the harden-parser branch from 792b920 to 7619537 Compare February 19, 2017 20:59

mreinstein approved these changes Feb 19, 2017

View reviewed changes

mreinstein merged commit 4853b74 into TooTallNate:master Feb 19, 2017

ZauberNerd deleted the harden-parser branch February 19, 2017 21:00

This was referenced Feb 20, 2017

Prevent Xcode errors #81

Closed

Emit empty string for nulls #80

Closed

fix empty <key /> lead index out of bounds issue #67

Closed

dpa99c mentioned this pull request Jan 16, 2019

feat: update plist depency tonpm repository dpa99c/cordova-custom-config#151

Closed

dpa99c added a commit to dpa99c/cordova-custom-config that referenced this pull request Jan 16, 2019

Switch to use [email protected] since TooTallNate/plist.js#79 is resolved by …

8a1000b

…TooTallNate/plist.js#84. Resolves #151

This was referenced Mar 26, 2021

[Snyk] Fix for 1 vulnerabilities saurabharch/Ionic-3-Taxi-booking-App-Similar-to-Uber-and-Taxify.-Contains-User-app-Driver-app-and-a-web-admin.#11

Open

[Snyk] Fix for 1 vulnerabilities Aarbel/web-app-to-stores#8

Open

This was referenced Feb 18, 2022

[Snyk] Fix for 1 vulnerabilities saurabharch/Ionic-3-Taxi-booking-App-Similar-to-Uber-and-Taxify.-Contains-User-app-Driver-app-and-a-web-admin.#16

Open

[Snyk] Fix for 1 vulnerabilities Aarbel/web-app-to-stores#13

Open

Aarbel mentioned this pull request Oct 5, 2022

[Snyk] Fix for 11 vulnerabilities Aarbel/web-app-to-stores#14

Open

taknuti mentioned this pull request Jan 16, 2023

Parsing a null-string and re-building that file results in incorrect structure #79

Closed

saurabharch mentioned this pull request Nov 27, 2023

[Snyk] Fix for 14 vulnerabilities saurabharch/Ionic-3-Taxi-booking-App-Similar-to-Uber-and-Taxify.-Contains-User-app-Driver-app-and-a-web-admin.#23

Open

Aarbel mentioned this pull request Nov 29, 2023

[Snyk] Fix for 11 vulnerabilities Aarbel/web-app-to-stores#17

Open

mkjekk mentioned this pull request Apr 15, 2024

[Snyk] Fix for 1 vulnerabilities mkjekk/copay#105

Open

mkjekk mentioned this pull request Apr 15, 2024

[Snyk] Fix for 1 vulnerabilities protokiller/copay#98

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Harden test-cases and align with other implementations #84

Harden test-cases and align with other implementations #84

ZauberNerd commented Feb 12, 2017 •

edited

Loading

ZauberNerd commented Feb 12, 2017

ZauberNerd commented Feb 19, 2017

mreinstein left a comment

mreinstein Feb 19, 2017

mreinstein Feb 19, 2017

mreinstein Feb 19, 2017

mreinstein Feb 19, 2017

mreinstein Feb 19, 2017

mreinstein Feb 19, 2017

mreinstein Feb 19, 2017

mreinstein left a comment

mreinstein Feb 19, 2017

ZauberNerd Feb 19, 2017

mreinstein Feb 19, 2017

ZauberNerd Feb 19, 2017

mreinstein Feb 19, 2017

mreinstein commented Feb 19, 2017

ZauberNerd commented Apr 11, 2017

mreinstein commented Apr 26, 2022

		@@ -10,6 +10,12 @@ var DOMParser = require('xmldom').DOMParser;

		exports.parse = parse;

		var NODE_TYPES = {

Harden test-cases and align with other implementations #84

Harden test-cases and align with other implementations #84

Conversation

ZauberNerd commented Feb 12, 2017 • edited Loading

ZauberNerd commented Feb 12, 2017

ZauberNerd commented Feb 19, 2017

mreinstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mreinstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mreinstein commented Feb 19, 2017

ZauberNerd commented Apr 11, 2017

mreinstein commented Apr 26, 2022

ZauberNerd commented Feb 12, 2017 •

edited

Loading