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

expired and failed cypher transactions #156

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dpisarewski
Copy link
Member

I also changed CypherResponse so that it retrieves all errors from response body.

@subvertallchris
Copy link
Contributor

Really nice! This didn't run on Travis for some reason. Could you push something (anything) to your branch to see if it'll submit now?

@dpisarewski
Copy link
Member Author

Oh sorry, there are merge conflicts.

@subvertallchris
Copy link
Contributor

Ah, right. I'm a little spoiled by pushing directly to branches on this repo, where it will always run at least one set of tests on Travis. :-)

Can you correct your merge conflicts so we can run tests before merge?

@dpisarewski
Copy link
Member Author

I added some examples into cypher_transaction_spec.rb. Why is the whole spec commented out? https://github.com/neo4jrb/neo4j-core/blob/master/spec/neo4j-server/unit/cypher_transaction_spec.rb

@subvertallchris
Copy link
Contributor

It was meant to be removed, I don't think it's a useful set of tests because of the intense amount of stubbing going on, it's too tied to implementation. I commented it out while working on the efficient_transactions branch and forgot to remove it.

@subvertallchris
Copy link
Contributor

Something to watch out for is tx = Neo4j::Transaction.new. Right now, the expectation is that tx would be the only transaction object, so you can do tx.failed? or tx.expired?. If new objects are created as the transaction's state changes, we'll need to delegate to those new objects.

@dpisarewski
Copy link
Member Author

When I run specs on master branch I get these errors:

Failures:

  1) Neo4j::Server::CypherTransaction#del deletes a node
     Failure/Error: tx.close
     Neo4j::Server::Resource::ServerException:
       Expected response code 200 Error for request http://localhost:7474/db/data/transaction/5437/commit, 404, 404
     # ./lib/neo4j-server/resource.rb:33:in `handle_response_error'
     # ./lib/neo4j-server/resource.rb:37:in `expect_response_code'
     # ./lib/neo4j-server/cypher_transaction.rb:46:in `_tx_query'
     # ./lib/neo4j-server/cypher_transaction.rb:38:in `_commit_tx'
     # ./lib/neo4j/transaction.rb:65:in `close'
     # ./spec/neo4j-server/e2e/cypher_transaction_spec.rb:185:in `Server'

  2) Cypher Queries CREATE (v1) RETURN ID(v1) returns correct response
     Failure/Error: response = HTTParty.send(:post, cypher_url, headers: resource_headers, body: body)
     NameError:
       uninitialized constant HTTParty
     # ./spec/neo4j-server/rest/create_node_spec.rb:14:in `(root)'

Am I missing something for the test environment?

@cheerfulstoic
Copy link
Contributor

I got those too, so I don't think it's you. Didn't have time to check it out when I looked earlier

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) when pulling a24194f on dpisarewski:invalid_transactions into aa69345 on neo4jrb:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) when pulling 9a69115 on dpisarewski:invalid_transactions into aa69345 on neo4jrb:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) when pulling 10f0c26 on dpisarewski:invalid_transactions into aa69345 on neo4jrb:master.

@subvertallchris
Copy link
Contributor

./spec/neo4j-server/rest/create_node_spec.rb can be deleted or rewritten. I don't know why it's using HTTParty directly. I'm not sure what it's trying to demonstrate that isn't already proven elsewhere.

@subvertallchris
Copy link
Contributor

Make sure you run the neo4j gem's master branch tests using your branch of core, too. It does a lot of transaction tests. That tx = Neo4j::Transaction.new thing is gonna pop up there.

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

4 participants