doanduyhai / Achilles

An advanced Java Object Mapper/Query DSL generator for Cassandra
http://achilles.archinnov.info
Apache License 2.0
241 stars 92 forks source link

Junit Part is hard to configure. #344

Closed AndreasMager closed 6 years ago

AndreasMager commented 6 years ago

I have added a method to access the startup parameters.

doanduyhai commented 6 years ago

Thanks for the PR. Can you also show an example of usage for this new changeStartupParameter ?

AndreasMager commented 6 years ago

In one of my project with achilles i need to access/set the cql port. I didn't find any easy solution to solve this problem.

@Rule
public AchillesTestResource<ManagerFactory> resource =
            AchillesTestResourceBuilder2.forJunit()
                                       .tablesToTruncate("foobar")
                                       .changeStartupParameter(c ->
                                           c.put(CASSANDRA_CQL_PORT, 9042)
                                       )
                                       .truncateAfterTest()
                                       .createAndUseKeyspace("test")
                                       .withScript("data_model.cql")
                                       .build((cluster, statementsCache) ->
                                           ManagerFactoryBuilder.builder(cluster)
                                                                    .doForceSchemaCreation(true)
                                                                    .withStatementsCache(statementsCache) 
                                                                    .withDefaultKeyspaceName("test")
                                                                    .build());

A getter for the ports in info.archinnov.achilles.embedded.ServerStarter could do the trick too.

doanduyhai commented 6 years ago

Ok I see the use case now.

Remark: if you don't set the CQL port, it will be randomized for you. Though, I can understand some scenarios where you want to set Cassandra params like CQL port or listen port.

My proposal: instead of creating the method changeStartupParameter which accepts a Consumer<TypedMap>, why don't you create instead a method like:

public AchillesTestResourceBuilder withCassandraParams(TypedMap cassandraParams) {
        this.cassandraParams.putAll(cassandraParams);
        return this;
}

Wouldn't it be more natural rather than injecting a Consumer ?

AndreasMager commented 6 years ago

I know the random port system and i like it. Sadly i can't use it in this project. Like i said if there was a getter for the port i could solve the problem.

You are right a Consumer is a bit weird.

What do you think about this snippet:

public AchillesTestResourceBuilder changeStartupParameter(Map<? extends String, ? extends Object> config) {
        cassandraParams.putAll(config);
        return this;
}

leads to:

@Rule
public AchillesTestResource<ManagerFactory> resource =
            AchillesTestResourceBuilder.forJunit()
                        .changeStartupParameter(Collections.singletonMap(CASSANDRA_CQL_PORT, 9042))
doanduyhai commented 6 years ago

I'm fine with the new code, however, the changeStartupParameter method name is weird for me. withCassandraParams sounds better because:

1) it's a Builder pattern so the method start with withXXX and does align with existing methods (withScript, withScriptTemplate)

2) the method impl is updating the variable cassandraParams so we make it clear in the method name that we're setting explicitly cassandra params (and not Achilles param for instance)

AndreasMager commented 6 years ago

I have changed the method signature and name. I also added an example in java doc.

doanduyhai commented 6 years ago

Excellent job! Thank you for your contribution