rdflib.js icon indicating copy to clipboard operation
rdflib.js copied to clipboard

Serialize in mocha

Open Neur0mante opened this issue 7 years ago • 17 comments

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

Neur0mante avatar Apr 07 '17 08:04 Neur0mante

@Neur0mante thank you!

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

dmitrizagidulin avatar Apr 07 '17 15:04 dmitrizagidulin

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" }

Neur0mante avatar Apr 07 '17 16:04 Neur0mante

@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).

dmitrizagidulin avatar Apr 07 '17 16:04 dmitrizagidulin

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

Neur0mante avatar Apr 07 '17 16:04 Neur0mante

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

Neur0mante avatar Apr 07 '17 16:04 Neur0mante

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

dmitrizagidulin avatar Apr 07 '17 16:04 dmitrizagidulin

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

Neur0mante avatar Apr 07 '17 16:04 Neur0mante

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"
  }
}

Neur0mante avatar Apr 07 '17 16:04 Neur0mante

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

dmitrizagidulin avatar Apr 07 '17 16:04 dmitrizagidulin

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"

Neur0mante avatar Apr 07 '17 16:04 Neur0mante

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 avatar Apr 07 '17 16:04 dmitrizagidulin

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

dmitrizagidulin avatar Apr 07 '17 16:04 dmitrizagidulin

(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.)

dmitrizagidulin avatar Apr 07 '17 16:04 dmitrizagidulin

for latest sdandardjs & chai see: https://github.com/feross/standard/issues/690#issuecomment-278533482

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'

elf-pavlik avatar Apr 08 '17 00:04 elf-pavlik

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 avatar Apr 08 '17 08:04 Neur0mante

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

Neur0mante avatar Apr 10 '17 12:04 Neur0mante

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

dmitrizagidulin avatar Apr 10 '17 21:04 dmitrizagidulin