Added integration test for 16 bit relationship-types #379

Closed
wants to merge 1 commit into from

5 participants

@jexp
Neo4j member

note that this test runs for 10 minutes or more

@jexp jexp Added integration test for 16 bit relationship-types
note that this test runs for 10 minutes or more
eacc8cb
@systay
Neo4j member

Can you share a bit of background around this? Why isn't it a unit test instead of a 10-minutes integration test?

@jexp
Neo4j member

Because it creates all possible rel-types to exhaust the space and show that it works up to 16 bit (and not further).

@jakewins
Neo4j member

Without having looked at the code, wouldn't it be possible to write it as a unit test that just tests the edge cases and some in-between values? This shouldn't need to run the whole stack right, just the parts that generate the ids, and perhaps separate tests for those components that need to suppor the edge cases as well (eg. storage and so on). Refactoring those parts of the kernel as necessary in order to write those tests.

A lot more work, I know, but adding 10 min to the build is a whole lot, and if we do it as unit tests (esp. if they drive some refactoring), we will be putting another dent towards improving the kernel :)

@thobe
Neo4j member

For the initial work we needed to verify that the full stack could handle full 16 bit relationship type ids.
The problem was actually NOT in the IdGenerators, but elsewhere in the code.

I agree that it would be better to identify all places that manipulate and/or store ids, and make sure that we have edge case testing there instead. When this test was written, it made more sense to test the entire stack, since we were pressed for time, but that is not the case anymore.

@tinwelint
Neo4j member

A note on length of this test. In 2.0 the mini-transactions that creates the relationship types are run "unforced" so creating 65k of these should be way faster. Also, property keys are created the same way here. We can get away with unforced since any other transaction using a created relationship type will be forcing anyway.

@jakewins
Neo4j member

One in-between way of doing this could be to write a test that patches the appropriate id generator to generate a small set of ids (like five or six), that span the full range of ids that we want to support.

Does anyone want to make the effort and write a test like that, or a set of proper edge-case unit tests? If so, I vote we keep this open, and update the PR when a new test is available. If nobody claims this, I will close the PR.

@jakewins jakewins closed this Feb 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment