Various tests improvements #18

paolochiodi · 2015-10-09T15:54:49Z

This PR includes:

make tests self contained, thus order independent
expand tests to fully cover specs as per https://github.com/rjrodger/seneca/blob/master/doc/data-store.md#cmdremove and discussions in the following issues: clarification on merge on update #13, Type of error returned by stores #14, remove command result #15, sql support #16 and behaviour for invalid skip and limit #17
adhere to "standard" style adopted for all seneca related repos
bring dependencies up to date

This PR does not include:

updates to the README
version bump
wrapped errors as discussed in Type of error returned by stores #14 (I'm working on an additional PR to bring that, but would like to get this merged first)

Notes:

instead of relying on test.sh to install seneca I added it to devDependencies. I'm not 100% sure it's the right place
this will require an update to how tests are run on the stores, since it now requires a "senecaMerge" option. That should be an instance of seneca with the store configured with the merge option set to false
tested with seneca from 0.5 to 0.7, node 0.10, 0.12, 4, 4.1
mem-store included with seneca fails some of the tests

paolochiodi · 2015-10-12T07:35:21Z

Please note that travis checks fail because the men-store used for the tests doesn't fully follow standards

geek · 2015-10-12T15:54:05Z

.travis.yml

@@ -1,4 +1,6 @@
 language: node_js
 node_js:
-  - "0.11"
+  - "4.1"


Can use "4" instead

do you think I should add "4" and also leave 4.1 and 4.0 since 4.2 has been released?

I would make it "4" to test against the current 4.x release.

mcdonnelldean · 2015-10-19T09:02:24Z

@paolochiodi Can you look at the failing tests? There are a couple of them.

mcdonnelldean · 2015-10-19T09:04:06Z

package.json

@@ -4,7 +4,7 @@
  "description": "Standard test cases for seneca stores",
  "main": "store-test.js",
  "scripts": {
-    "test": "./test.sh"
+    "test": "lab test/*.test.js -r console -v -L"


You might want to set the timeout here to 3000 we have found travis can be slow at times and cause tests to fail due to overrun. 3000 seems to be the sweet spot.

paolochiodi · 2015-10-19T09:09:48Z

@mcdonnelldean tests on this repot aren't real tests
They actually use the tests in this module to test the mem-store built in seneca (which I think is considered a "reference implementation".

I've introduced some new test to better cover the data entities specification, and some them are failing because even men-store doesn't fully and correctly implement that. In order to have all tests green we should amend the mem-store

mcdonnelldean · 2015-10-19T09:23:54Z

@paolochiodi super, as discussed let's fix up mem store while we wait for @rjrodger to review.

geek · 2015-11-03T14:35:23Z

@paolochiodi this change looks good... I would like to get the changes to mem-store in first so that we can have all green tests. I went ahead and merged in #22, so you will need to pull down those changes and merge :/

paolochiodi · 2015-11-10T11:58:54Z

I've rebased to incorporate latest changes in master branch.
I've also updated seneca and seneca-men-store to make all tests pass (relevant PR here senecajs/seneca#217 and here senecajs/seneca-mem-store#36)

Releasing these will require some coordination and some work on package.json to highlight that these spec changes:

break existing spec and stores
require latest versions of seneca (the ones including follow new store specs seneca#217)

Also cover more of the specs

Fixes #15

see #14 (comment)

paolochiodi · 2015-11-13T10:56:47Z

Rebased to bring this up-to-date with master

mirceaalexandru · 2015-11-19T09:49:18Z

store-test.js

-      scratch.foo2.p2 = 'v2'
+      it('should allow to not merge during update with merge$: false', function (done) {
+        var foo = si.make('foo')
+        foo.id = 'to-be-updated'


@paolochiodi I think is better not to define id values in these tests. Some DB engines have rules about ids - like MongoDB that uses an ObjectId. So in order to use these tests as a standard maybe we should not have id values defined in these tests.

The id can be retrieved from callback of the save action.

I guess this also relates to senecajs-labs/seneca-shard-store#11
On a general note I'm ok to rewrite tests in a way that createEntities will store auto-generated ids for later uses (i.e.: when we want to update the entities, like this case).

I begin to feel that at this point may be better to remove the possibility to specify the id on create from the specs, if many of the DB engines can't support them. What do you think?

I am the supporter of the idea that we should have something like this:

define a set of functions that all stores will try to follow - the "standard" store functions that we have now

support in the stores as many as possible "standard" functions offered by the DB engines. Here is also this function of allowing to specify the id or the entity to be saved if that is supported.

clearly specify in the documentation when a certain store do not have support for a "standard" function.

So, from my point of view, if Riak do not support list$ is not a problem, but we should clearly specify this in its documentation. The developers that will use that store probably know already this thing.

I also think that the benefit of having a functional store with all necessary tools offered by a specific DB engine is greater than limiting all stores to the smallest set of common functions offered by all DB engines.

Shard-store can also have a list of specific functions that should not be used by developers if they want to have it run correctly - this should not limit the actual functions implemented in the used stores.

I agree on the idea of having a basic set of standard functions, but I think all the stores must support that minimal set.

The entity framework is quite minimal and doesn't add much to the db driver beside a common api, I don't see the point of having a store for a db that can't support that api: sometimes may be better to directly go with the driver or a different type of abstraction (e.g: https://github.com/AdrianRossouw/seneca-knex-store)

Maybe @rjrodger and @geek can weight in their opinion?

Also let me know how to proceed with this PR, if I need to split the tests into different "levels" of support

Tests improvements

geek reviewed Oct 12, 2015
View reviewed changes

paolochiodi mentioned this pull request Oct 19, 2015

[WIP] Reorganize and refactor senecajs/seneca-mysql-store#20

Closed

mcdonnelldean reviewed Oct 19, 2015
View reviewed changes

paolochiodi mentioned this pull request Nov 3, 2015

Annotations #22

Merged

This was referenced Nov 10, 2015

Update to specs senecajs/seneca-mem-store#36

Merged

follow new store specs senecajs/seneca#217

Merged

paolochiodi mentioned this pull request Nov 12, 2015

Add tests for {load$: true} params when remove$ is executed #23

Closed

paolochiodi added 15 commits November 13, 2015 11:39

Refactor to be order independant

c5c9097

Also cover more of the specs

Fix remove behaviour according to #15

aed8a12

Fixes #15

proper merge behaviour

c074ed3

tests for mandatory merge option

13397ab

allow null or undefined for missing attributes

eb5258c

change sql tests style

2ec67ae

cleanup and uniform style

aa186e5

follow the standard style

ce9bb7d

update dependencies and add proper seneca dependencies

72279e7

remove unneded test runner

7f48943

add tests for invalid skip and limit

3a87ff1

make updates work like upserts

c2d55fd

see #14 (comment)

support opaque ids on list

acc3a65

tests for common save mistakes

c416e7d

test for native support

38ace29

paolochiodi added 7 commits November 13, 2015 11:55

add tests around attributes mixing between entity and query

00acb82

fix error on id

21128d3

assert correct behaviour for load$, list$ and remove$ without query

d2c106e

better test for no query on load$

577434d

fix typo

8e460bd

use seneca merge instance to setup tests

0592375

Make sure merge$ command doesn't leak

d3bfcf2

mirceaalexandru reviewed Nov 19, 2015
View reviewed changes

geek self-assigned this Nov 24, 2015

update seneca dependency and explicitly require mem store

bd32ad9

paolochiodi added a commit that referenced this pull request Dec 10, 2015

Merge pull request #18 from paolochiodi/follow-specifications

eaf0f48

Tests improvements

paolochiodi merged commit eaf0f48 into senecajs:master Dec 10, 2015

This was referenced Dec 10, 2015

add test checking modification of data #11

Closed

Add test for modifications to entity after save completes #10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various tests improvements #18

Various tests improvements #18

paolochiodi commented Oct 9, 2015

paolochiodi commented Oct 12, 2015

geek Oct 12, 2015

paolochiodi Oct 13, 2015

geek Oct 13, 2015

mcdonnelldean commented Oct 19, 2015

mcdonnelldean Oct 19, 2015

paolochiodi commented Oct 19, 2015

mcdonnelldean commented Oct 19, 2015

geek commented Nov 3, 2015

paolochiodi commented Nov 10, 2015

paolochiodi commented Nov 13, 2015

mirceaalexandru Nov 19, 2015

paolochiodi Nov 19, 2015

mirceaalexandru Nov 19, 2015

paolochiodi Nov 19, 2015

Various tests improvements #18

Various tests improvements #18

Conversation

paolochiodi commented Oct 9, 2015

paolochiodi commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdonnelldean commented Oct 19, 2015

Choose a reason for hiding this comment

paolochiodi commented Oct 19, 2015

mcdonnelldean commented Oct 19, 2015

geek commented Nov 3, 2015

paolochiodi commented Nov 10, 2015

paolochiodi commented Nov 13, 2015

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