search

neo4j / docker-neo4j

Docker Images for the Neo4j Graph Database
Apache License 2.0
331 stars 172 forks source link

5.x dump test uses the new dump command semantics #361

Closed PetrJanouch closed 2 years ago

PetrJanouch commented 2 years ago

@MishaDemianenko Why? The test should be used only for 5.x images. 4.x has another test. At least to my understanding.

MishaDemianenko commented 2 years ago

@MishaDemianenko Why? The test should be used only for 5.x images. 4.x has another test. At least to my understanding.

not in this repo. Check for example https://github.com/neo4j/docker-neo4j/blob/master/src/test/java/com/neo4j/docker/neo4jadmin/TestBackupRestore.java and TestBackupRestore44 over there. There are checks like Assumptions.assumeTrue( TestSettings.NEO4J_VERSION.isAtLeastVersion( Neo4jVersion.NEO4J_VERSION_500 ) and as you can see old versions are executed on top of your PR

MishaDemianenko commented 2 years ago

@jennyowen can correct me if i'm wrong and something changed here (but not according to tesst code) ^^^

PetrJanouch commented 2 years ago

That is really weird. the test I fixed already uses 5.x-only command semantics. It uses neo4j-admin database dump, which is something 4.x will not understand.

MishaDemianenko commented 2 years ago

hm looks like that test class TestDumpLoad has this thing Assumptions.assumeTrue( TestSettings.NEO4J_VERSION.isAtLeastVersion( Neo4jVersion.NEO4J_VERSION_500 ), as part of beforeAll so version protection should be covered but that i suppose and we should be good

MishaDemianenko commented 2 years ago

@MishaDemianenko Why? The test should be used only for 5.x images. 4.x has another test. At least to my understanding.

not in this repo. Check for example https://github.com/neo4j/docker-neo4j/blob/master/src/test/java/com/neo4j/docker/neo4jadmin/TestBackupRestore.java and TestBackupRestore44 over there. There are checks like Assumptions.assumeTrue( TestSettings.NEO4J_VERSION.isAtLeastVersion( Neo4jVersion.NEO4J_VERSION_500 ) and as you can see old versions are executed on top of your PR

Whole class is already under version check so we should be good

jennyowen commented 2 years ago

Yep, that test is 5.0 for reasons you already figured out. Looks good!