RepoSense icon indicating copy to clipboard operation
RepoSense copied to clipboard

RepoConfigurationTest: Simplify code in `repoConfig_ignoresStandaloneConfig_success` test case

Open anubh-v opened this issue 5 years ago • 3 comments

The code snippet below shows the repoConfig_ignoresStandaloneConfig_success test case. In lines 153 - 157, we parse some input to create a list of RepoConfiguration objects.

https://github.com/reposense/RepoSense/blob/a2adb592431b2ee59fb3d22a3f6c8ab703870de9/src/test/java/reposense/model/RepoConfigurationTest.java#L130-L164

The lines 153 - 157 can be replaced with the line below:

List<RepoConfiguration> actualConfigs = RepoSense.getRepoConfigurations((ConfigCliArguments) cliArguments);

It seems that the lines 153 - 157 are duplicating the same work done in the getRepoConfigurations method of the RepoSense class.

Please let me know if I'm misunderstanding something here.

anubh-v avatar Dec 10 '19 16:12 anubh-v

Yes, it is duplicating some parts of the method. However, I believe it is necessary.

  1. This is the unit test for RepoConfiguration. Apart from the method directly from the class itself, we should use the minimum outside method. Even if we have to use, we should use the lower abstraction one. So, the change of other classes and other methods will infect this unit test minimally. A unit test is meant to test the behavior of this specific class.
  2. Looking to the method itself, also it is longer than just use RepoSense::getRepoConfigurations, however, it only uses highly related one and the class method. RepoSense::getRepoConfigurations is an outsider method with high abstraction, we certainly do not want the change of this method will affect the unit test of other models.

fzdy1914 avatar Dec 11 '19 12:12 fzdy1914

Thanks for the response @fzdy1914.

It makes sense that it may not be a good idea to use RepoSense::getRepoConfigurations in a test because future changes to this method would affect the test.

In that case, perhaps we should edit the repoConfig_ignoresStandaloneConfigInCli_success test case, since it uses the RepoSense::getRepoConfigurations method ? (see line 179 in snippet below)

https://github.com/reposense/RepoSense/blob/a2adb592431b2ee59fb3d22a3f6c8ab703870de9/src/test/java/reposense/model/RepoConfigurationTest.java#L167-L186

anubh-v avatar Dec 12 '19 16:12 anubh-v

@anubh-v True, it is better not to use this method. When I wrote this method previously, I should forget to take it into account.

However, it may be an overkill to create a separate PR for just fixing this issue. Maybe you can consider sneaking in the code when toughing this file in other related PR.

fzdy1914 avatar Dec 13 '19 03:12 fzdy1914