Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding record version and data to VERSION_EXISTS error #1 #137

Merged
merged 3 commits into from
May 17, 2016

Conversation

yasserf
Copy link
Contributor

@yasserf yasserf commented May 6, 2016

No description provided.

*
* @public
*/
RecordTransition.prototype.sendVersionExists = function( socketWrapper, version ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused: How do we know the record's version before it has been loaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was based on the previous logic, which is that the last version in the steps ( updates added ) is assumed to be the records current version. I'm guessing this was to assume a positive path ( which is usually correct ).

@yasserf yasserf force-pushed the feature/#1versionconflicts branch from ad1a1f8 to 3e938bf Compare May 9, 2016 18:13
recordHandler.handle( clientB, {
raw: msg( 'R|U|existingRecord|7|{"name":"Kowalski"}' ),
topic: 'R',
action: 'U',
data: [ 'existingRecord', 7, '{"name":"Kowalski"}' ]
});

setTimeout(function(){
expect( clientA.socket.lastSendMessage ).toBeNull();
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WolframHempel Be good to look at this behaviour / tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on it

@@ -75,11 +75,12 @@ describe( 'records are requested from cache and storage sequentually', function(
expect( options.storage.lastRequestedKey ).toBe( 'doesNotExist' );
});

it( 'fails gracefully if an error occured out of order', function( done ){
//TODO: yasserf: I don't get this test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it destroys the record request in between getting data from the cache and from the storage - which is not supposed to happen...but just in case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant more that done is called in the record request and and after 20 milliseconds, which means it can exit successfully without any expectations run

@yasserf yasserf force-pushed the feature/#1versionconflicts branch from 9f6e92e to 8c41841 Compare May 13, 2016 17:12
@WolframHempel WolframHempel mentioned this pull request May 14, 2016
@WolframHempel WolframHempel merged commit 5bf38c5 into master May 17, 2016
@yasserf yasserf removed the in review label May 17, 2016
@yasserf yasserf deleted the feature/#1versionconflicts branch May 19, 2016 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants