dash in collection names (2)

[mmouterde]

This PR contains:

• A BUGFIX

collection names should not contains '-' otherwise database.remove() fails.

[pubkey]

this is because the database/collection can be created lazy. You could run any query here instead, then you can be sure that everything is initialized.

[mmouterde]

ok, I got it.

We may expose the internal promise on RxStorage interface to have an explicit way to wait for the lazy creation ? I guess all storage plugins may need this king of lazy loading promise ?

Playing with "fake querie" or wait(100) seems to me quite obscur, specially in unit tests (more than the simple unit test performance issue).

[pubkey]

You should remove this return when you want to prove that a bug exists.
This makes it easier to check the CI for any hints on what goes wrong.

[mmouterde]

I would be great but how to deal with storage.settings.indexedDB.internalDatabase._databases.keys() in assertion for other storages ?

[pubkey]

Ah I see. That makes sense.

[pubkey]

Would this proposed fix also work when both, the database and the collection have a dash in it?

[mmouterde]

Nop, in removeRxDatabase the database name has already been removed.
This is probably another issue

[pubkey]

I think I understood the problem now.
I shouldnt have used the dash char to merge database- and collection name.

Inside of the removeRxDatabase function, we know the database name.
So to get the collection name, we should strip the databasename from the colDoc.key (and also the first dash after that.).
By doing so, we can be sure that both, the database and the collection name can contain a dash.

[mmouterde]

In removeRxDatabase the database name has already been removed.
According to the following capture, there is a suffix (version of schema ??) added after the collectionName.

We may use colDoc.data.name ?

[pubkey]

We may use colDoc.data.name ?

Yes, that sound right.

[mmouterde]

I finally add sinon library to be able to assert without using internal specification.
Let me know if it's ok for you. I'm sure you will get the huge benefit of it ^^

[pubkey]

I'm sorry but I really do not want to add another test dependency just to reproduce this single problem. I do not understand it and it adds more complexity.
For example this lines:

spy.getCalls()
                .reduce((acc, call) => {
                    return acc.includes(call.args[0].collectionName) ? acc : [...acc, call.args[0].collectionName]

I have no idea what this does.

Can't we just reopen the database and check if the data was really deleted?

[mmouterde]

I understand your reticences.

I am convinced that the whole project could benefit from this lib (for test purpose). Especially for a such project with abstraction layers and plugins ! You really could test the genetic layer (ie rx-database) independently of any storage or plugin.

I could introduce you the piece of code you mentioned. If you are not confident with this kind of tool please, tell me ! I could help.

• const spy = sinon.spy(storage, 'createStorageInstance');

This will create a proxy for the createStorageInstance method of the given storage instance.
From this point, all calls of createStorageIntacne made are reported to the spy (including info such arguments, results, exceptions).

It could be beemed as the following pseudo code :

const storage = getStorage();
const spy = [];
storage.createStorageInstance = ()=> {
  spy.push(arguments)
  return storage.createStorageInstance().call(storage.arguments)
}
functionThatUseStorage(storage);
assertOnSpyContent(spy.containst('name_'))

Back to our example you mentioned, the storage is spied before being passed to createRxDatabase so each call to this createStorageInstance method on the storage instance could be reported by the spy.

spy.getCalls() returns the calls list made to createStorageInstance by the code behind createRxDatabase and remove. Actually all calls made on the storage instance from the line sinon.spy(storage, 'createStorageInstance'); to the line spy.getCalls(). My peudo code shows that we can keep a eye on arguments by SinonJs keep more than simply arguments :

A reported call looks like

{ 
   args:[], // All arguments passed to the spied method for the given call
   exception:[], // the exeption thrown if any
   returnValue:{} // the returning value if any
    ... and more
   }

In our case, I'm interested in the collectionName passed as the first argument of createStorageInstance({collectionName:""})
So I get this info from the call call.args[0].collectionName. This means that I want to retrieve the collectionName passed as first arguments (arg[0]) for each call.

A agree, this looks probably cryptic (reduce() function often imply a bad readability according to me).
By the way I want to map a list of spy calls to the list of collectionName provided and make assertions on this list.
To make is more robust and futur proof I filtered the collectionName list to those starting with 'name_' to avoid all internal stuff. I also removed duplicates.

So from the list of calls :

const calls = [
  { args:[ { collectionName: 'name_1'} ]},
  { args:[ { collectionName: 'name_1'} ]},
  { args:[ { collectionName: 'name_2'}]},
  { args:[ { collectionName: 'internalStull'}]}
]`;

I got :

const collectionNamesPassed = ['name_1','name_2']

I hope make more sense for you now. Please tell me If understanding this is still difficult even with this short demonstration.

As you could see, by this way, my test could apply for all storages, all environments. I could removed limitations for this test (the isDexie and isNode initially required)

Anyway, you are the boss, it's your project so, your choices ^^

[pubkey]

I know these mocking libraries. I think they lead people to writing untestable code together with unreadable tests.
In the end it is javascript, so if you want to really check how a function is called, you can overwrite storage.createStorageInstance() with your own method.

But your test has to prove that a RxCollection is deleted. So checking for an internal call to storage.createStorageInstance() should not be part of the test.
I mean there is a way you expect RxDB to behave, which is wrong. And this wrong behavior should be reproduceable with just plain public RxDB methods, not with internal stuff.

[mmouterde]

Hum, ok, up to you. On my opinion, we should test implementation layer by layer unless these are only functional tests.

RxDb is made of layers, with public interfaces.
I used public interface of RxDatabase, mock public interface RxStorage.
I was trying to test this specified bug on the RxDatabase implementation which could be detected on the storage layer (whatever their both implementation).

I have done my best to make tests independent from storage implementation (specially for assertion).

Please let me know you would done on your side. I always ready to learn from others !

[pubkey]

I was trying to test this specified bug on the RxDatabase implementation which could be detected on the storage layer (whatever their both implementation).

I understand now. Reproducing the problem with plain RxDB API is not possible.

I now changed the returned value from removeRxDatabase so that the caller can known which collections have been removed and it is properly testable.