Serialize in mocha #180

Neur0mante · 2017-04-07T08:45:00Z

I wanted to convert the serialize tests to mocha, and while doing that I had to perform some fixes here and there so this ended up as quite a wide PR.
List of modifications:

Added a .eslintrc configuration file extending standard. I've tried to specify the rules so it would have the least impact with existing code. I think it would be a good idea to have this shared in the github.
Moved all the files we are using for the serialization tests in sample_files folder for cleanliness.
There are a few changes to the fetcher. Most were brought up by the linter.
There were various instances of

if (sf.fetchCallbacks[xhr.resource.uri]) {
                  if (!sf.fetchCallbacks[newURI]) {
                    sf.fetchCallbacks[newURI] = []
                  }
                  sf.fetchCallbacks[newURI] === sf.fetchCallbacks[newURI].concat(sf.fetchCallbacks[xhr.resource.uri])
                  delete sf.fetchCallbacks[xhr.resource.uri]
                }

I don't think the triple equal is intended am I missing something?

I added a check for windows so that we feed the xhr with two slashes URIs while keeping three slashes in the kb. It's still possible to insert file URIs with two slashes, but it's no longer necessary to make it work correctly under windows.

      if (process) {
        if (process.platform.slice(0, 3) === 'win') {
          actualProxyURI = actualProxyURI.replace(/\/\/\//g, '//')
        }
      }

I've written a test helper which incorporates the functionalities from data.js operating mostly using promises so it can be used asynchronously under mocha.
I've converted all the current serialization tests into mocha. They are all under the serialization.spec.js file. They work under win and Linux, they will show diffing in case of errors, they will exit with error in case of a mismatch.
This is a pretty bare conversion but there are some more accurate testing for the loaded files in it.
chai-as-promised allow to work seeminglessly with promises so it's pretty clean and it should all run asynchronously so the suite is relatively fast overall.
I made the 9th test work through promises since the json+ld converter is async

The first 8 tests has been ported. The reference and source files has been moved into sample_files folder

The converter for json+ld files is async and require a callback a function to work. Luckily we just needed to work around it with promises in the test.helper. T11, 12, 13 have problems with BlankNodes ids. This was related to async code not being inside "it"s

Don't place anything outside "it" in asynchronous testing, or it will be all jammed together with unpredictable consequences. Place all your code inside its. trying to solve test 13 Fixed linux testing Fixed t13

Removed all typescript dependencies. Parallelized all tests. Tidied up the serializaton test folder. Introduced and setted eslint with config file. Uri should now be consistent in windows and Linux. You can still add "wrong" uris, but you don't have to to make xhr work under windows.

dmitrizagidulin · 2017-04-07T15:04:27Z

@Neur0mante thank you!

Tell us more about the .eslintrc extensions -- what's the intended effect?

dmitrizagidulin · 2017-04-07T15:05:56Z

package.json

    "n3": "^0.4.1",
    "xmldom": "^0.1.22",
-    "xmlhttprequest": "^1.7.0"
+    "xmlhttprequest": "1.8.0",
+    "bluebird": "3.5.0"


Why the usage of Bluebird, instead of native promises?

dmitrizagidulin · 2017-04-07T15:07:23Z

package.json

@@ -30,9 +30,11 @@
  "dependencies": {
    "async": "^0.9.x",
    "jsonld": "^0.4.5",
+    "mz": "^2.6.0",


Since you're only using mz in tests, this should go in devDependencies. Also, I'd prefer it if you used fs-extra instead.

I'm using mz for the promisified version of the load and write. We could promisify fs-extra by:

const Promise = require('bluebird') const fs = Promise.promisifyAll(require('fs-extra'))

(which I think was probably the reason why I started to use bluebird promises in the first place)

Yeah, I do hate that fs-extra doesn't offer a native promise-based interface (and instead outsources that to promisify). :(
Ok, so, might as well keep mz (though move it to devDependencies), and remove the Bluebird dep.

dmitrizagidulin · 2017-04-07T15:08:13Z

package.json

    "diff": "^3.2.0",
+    "eslint": "^3.18.0",


What are these for? The standard dep pulls these in, usually.

I wasn't sure if you could define .eslintrc files by using the standard rep so I was doing it backwards by extending the standard-plugin, but yeah I can reverse that.

dmitrizagidulin · 2017-04-07T15:14:49Z

tests/serialize/sample_files/diff.js

-
+  data1 = data1.replace(/\r\n?/g, '\n')
+  data2 = data2.replace(/\r\n?/g, '\n')
+  console.log(data1)


Stray console.log()?

dmitrizagidulin · 2017-04-07T15:17:43Z

src/fetcher.js

@@ -585,17 +585,18 @@ var Fetcher = function Fetcher (store, timeout, async) {
  // Returns promise of XHR
  //
  //  Writes back to the web what we have in the store for this uri
-  this.putBack = function (uri, options = {}) {


Why the removal of default arguments here and on line 597?

I thought this came from a linter warning but it actually probably just came from a diff with an older version of the fetcher.

Neur0mante · 2017-04-07T16:02:21Z

I'll break down the .eslintrc here

Of these only mocha is actually necessary:
"env": {
"node": true,
"es6": true,
"mocha": true
},
it's worth to point out that this doesn't add support for chai, and the plugins I've found weren't completely satisfying, so I'm using
/* eslint no-unused-expressions:0 */
inside the tests. This must be enforced cause these
testHelper.kb.statementsMatching(undefined, undefined, lit).should.not.be.undefined
are considered unused.

These are for the tabulator plugin for firefox that pop out here and there:

"globals": {
    "tabulator": true,
    "Components": true
  }

is it still supported by the way? Otherwise we could remove all those checks.

rules": {
standard doesn't enforce a particular spacing in arrays but git/github obviously complains about inconsistencies. This will just place spaces at the start and end of all arrays.

    "array-bracket-spacing": [
      "error",
      "always"
    ],

There are so many variable/classes that are not camelcased that this is mostly an annyoiance. Same thing with unused vars.
"camelcase": "
"no-unused-vars": "warn"
}

dmitrizagidulin · 2017-04-07T16:07:09Z

@Neur0mante re .eslintrc -- all of that stuff is handled by standard, so we don't need that file (or the various eslint deps and plugins).

Neur0mante · 2017-04-07T16:16:28Z

Sorry I'm used to ctrl+enter to insert a new line from everywhere I wasn't actually done breaking it down

Neur0mante · 2017-04-07T16:22:52Z

es6 is also needed, I'm testing without it and it's complaining about const, import etc...

dmitrizagidulin · 2017-04-07T16:26:08Z

What's complaining about const/import etc? Standard is?

Neur0mante · 2017-04-07T16:26:50Z

nevermind. It was because I removed the extends="standard" from the .eslintrc

Neur0mante · 2017-04-07T16:28:22Z

This should do

{
  "extends": "standard",
  "env": {
    "mocha": true
  },
  "globals": {
    "tabulator": true,
    "Components": true
  },
  "rules": {
    "array-bracket-spacing": [
      "error",
      "always"
    ],
    "camelcase": "off",
    "no-unused-vars": "warn"
  }
}

dmitrizagidulin · 2017-04-07T16:29:43Z

Walk me through it, though. What is the .eslintrc for, over out of the box standard?

Neur0mante · 2017-04-07T16:47:39Z

Without the mocha env describe and it get flagged as global variables (which are forbidden in standard)
it's worth to point out that this doesn't add support for chai, and the plugins I've found weren't completely satisfying, so I'm using
/* eslint no-unused-expressions:0 */
inside the tests. This must be enforced cause these
testHelper.kb.statementsMatching(undefined, undefined, lit).should.not.be.undefined
are considered unused.

Same problem here, these are for the tabulator plugin for firefox that pops out here and there:

"globals": {
    "tabulator": true,
    "Components": true
  }

is it still supported by the way? Otherwise we could remove all those checks.

rules": {
standard doesn't enforce a particular spacing in arrays but git/github obviously complains about inconsistencies. This will just place spaces at the start and end of all arrays.

    "array-bracket-spacing": [
      "error",
      "always"
    ],

There are so many variable/classes that are not camelcased that this is mostly an annyoiance. Same thing with unused vars.

"camelcase": "off"
"no-unused-vars": "warn"

dmitrizagidulin · 2017-04-07T16:52:35Z

The mocha env part and the warnings about the globals are handled in the standard: globals section of package.json.

In the case of testHelper.kb.statementsMatching(undefined, undefined, lit).should.not.be.undefined considered unused -- which part of it does it complain about being unused? We're widely using chai with standard across most our libs, and hasn't been a problem.

👎 to enforcing array spacing, or silencing unused warnings.

dmitrizagidulin · 2017-04-07T16:54:04Z

To clarify, you should instead add "it" and "describe" to the standard: globals: section of package.json.

dmitrizagidulin · 2017-04-07T16:55:27Z

(I see what you mean about camelcasing warnings, there are a bunch of snake-cased variables in this library due to it being transpiled from Python.)

elf-pavlik · 2017-04-08T00:33:19Z

for latest sdandardjs & chai see: standard/standard#690 (comment)

I ended up using it this way

// expect.js

import chai from 'chai'
import dirtyChai from 'dirty-chai'

chai.use(dirtyChai)
export default chai.expect

// fooTest.js

import expect from './expect'

Neur0mante · 2017-04-08T08:47:26Z

That looks good, turning all those calls into actual functions makes more sense. With should it plugs in automatically with chai.should() right? It seems to be working.

"no-unused-vars": "warn" was just turning those from errors to warning, but it's not a big issue.
I might work on refactoring the python library out, it doesn't look too hard.

Neur0mante · 2017-04-10T12:19:21Z

It should be fine now.
The correction on the triple equals in the fetcher is fine right?

dmitrizagidulin · 2017-04-10T21:13:10Z

@Neur0mante I think the correction is fine, yes (re triple equals)

First tests in mocha, moving test files

c173b4d

The first 8 tests has been ported. The reference and source files has been moved into sample_files folder

Neur0mante force-pushed the serialize-in-mocha branch 5 times, most recently from 7433fe0 to 5353a06 Compare April 7, 2017 12:12

Neur0mante added 5 commits April 7, 2017 14:15

Making t9 work

5858b59

The converter for json+ld files is async and require a callback a function to work. Luckily we just needed to work around it with promises in the test.helper. T11, 12, 13 have problems with BlankNodes ids. This was related to async code not being inside "it"s

Trying to make the URI identical in win and unix

26d7dc6

Pay Attention To This (tests fixed)

dae277b

Don't place anything outside "it" in asynchronous testing, or it will be all jammed together with unpredictable consequences. Place all your code inside its. trying to solve test 13 Fixed linux testing Fixed t13

Full es6 version of helper and spec

2eb096a

Neur0mante force-pushed the serialize-in-mocha branch from 5353a06 to 2eb096a Compare April 7, 2017 12:17

dmitrizagidulin reviewed Apr 7, 2017

View reviewed changes

Standard, dirty-chai, cleaning

50d06f1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialize in mocha #180

Serialize in mocha #180

Neur0mante commented Apr 7, 2017 •

edited

dmitrizagidulin commented Apr 7, 2017

dmitrizagidulin Apr 7, 2017

dmitrizagidulin Apr 7, 2017

Neur0mante Apr 7, 2017

dmitrizagidulin Apr 7, 2017

dmitrizagidulin Apr 7, 2017

Neur0mante Apr 7, 2017

dmitrizagidulin Apr 7, 2017

dmitrizagidulin Apr 7, 2017

Neur0mante Apr 7, 2017 •

edited

Neur0mante commented Apr 7, 2017 •

edited

dmitrizagidulin commented Apr 7, 2017

Neur0mante commented Apr 7, 2017

Neur0mante commented Apr 7, 2017

dmitrizagidulin commented Apr 7, 2017

Neur0mante commented Apr 7, 2017

Neur0mante commented Apr 7, 2017

dmitrizagidulin commented Apr 7, 2017

Neur0mante commented Apr 7, 2017 •

edited

dmitrizagidulin commented Apr 7, 2017 •

edited

dmitrizagidulin commented Apr 7, 2017

dmitrizagidulin commented Apr 7, 2017

elf-pavlik commented Apr 8, 2017 •

edited

Neur0mante commented Apr 8, 2017

Neur0mante commented Apr 10, 2017

dmitrizagidulin commented Apr 10, 2017

Serialize in mocha #180

Are you sure you want to change the base?

Serialize in mocha #180

Conversation

Neur0mante commented Apr 7, 2017 • edited

dmitrizagidulin commented Apr 7, 2017

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

Neur0mante Apr 7, 2017 • edited

Choose a reason for hiding this comment

Neur0mante commented Apr 7, 2017 • edited

dmitrizagidulin commented Apr 7, 2017

Neur0mante commented Apr 7, 2017

Neur0mante commented Apr 7, 2017

dmitrizagidulin commented Apr 7, 2017

Neur0mante commented Apr 7, 2017

Neur0mante commented Apr 7, 2017

dmitrizagidulin commented Apr 7, 2017

Neur0mante commented Apr 7, 2017 • edited

dmitrizagidulin commented Apr 7, 2017 • edited

dmitrizagidulin commented Apr 7, 2017

dmitrizagidulin commented Apr 7, 2017

elf-pavlik commented Apr 8, 2017 • edited

Neur0mante commented Apr 8, 2017

Neur0mante commented Apr 10, 2017

dmitrizagidulin commented Apr 10, 2017

Neur0mante commented Apr 7, 2017 •

edited

Neur0mante Apr 7, 2017 •

edited

Neur0mante commented Apr 7, 2017 •

edited

Neur0mante commented Apr 7, 2017 •

edited

dmitrizagidulin commented Apr 7, 2017 •

edited

elf-pavlik commented Apr 8, 2017 •

edited