lightbend / mesos-spark-integration-tests

Mesos Integration Tests on Docker/Ec2
16 stars 9 forks source link

Consolidate the tests in one class #14

Closed skyluc closed 9 years ago

skyluc commented 9 years ago

Based on #12

I will rebase on the other PRs when merged, but please review now.

skonto commented 9 years ago

I think there are two cases where we would like not to have one class for both cluster and client mode in terms of design: a) we have created many tests so this class is getting huge b) we need to have specific testing for cluster vs client modes. Then by context separation comes naturally. Btw what does Int mean in MesosIntSpec name (integer tests)? Other than the above which may not be a concern now LGTM.

skyluc commented 9 years ago

@skonto, the problem is that code duplication is bad, and the current code is pretty bad because of the code duplication. Some code has been improved in one side, not on the other, ...

I'll rework it. We can split the test in different traits, split along group of test cases. Then the client and cluster test classes will be basically empty, extending the different traits defining the tests.

skonto commented 9 years ago

From a macro perspective the only difference is the mode boolean value passed at the runner along with the if statement. We could have one trait with common tests and one for cluster mode. So instead we could create a new dynamically with the appropriate mix-in (eg. predefined types). Is this the idea you have? From a micro perspective within each test we have similar functionality like spark ops eg rdd.sum etc which is the main test code and have different parameters or assertions for cluster vs client mode. Abstracting the test code seems ok, everything else may be too restrictive if that dictates a way to write test. Is there some implicit way from scalatest to deactivate certain tests when running one or the other mode while keeping consolidation of tests? If the list of tests grows big we can always split with some criteria.

skyluc commented 9 years ago

Yes, the macro level idea. So we don't have one class with all the tests, but we don't duplicate the test code for each model. Duplication inside the tests in not great, but not too bad at this point.

skonto commented 9 years ago

+1

dragos commented 9 years ago

LGTM