Change how ConfigSystemTest is done by using RepoSense::main

[chan-j-d]

What change(s) would you like to see in RepoSense?

Change how system tests are run by using the RepoSense::main method to provide full end-to-end validation through supplying CLI arguments and comparing the output JSON files.

Is the feature request related to a problem?
Adding new system tests can be difficult as the existing methods use the config files in the resources folder by default. This makes it difficult to perform tests on more specific scenarios such as using a new config file to test against a specific branch of a specific repo only.

The current setup with the ConfigSystemTest::generateReport method also bypasses the parsing of the original CLI arguments and setting up of RepoConfigurations. If the main method is changed, then this method would also have to be changed with it. I believe it would be beneficial to use RepoSense::main instead for the full end-to-end validation.

Setting up test cases would then be similar to specifying the exact CLI arguments that we would normally run against the RepoSense.jar file.

If possible, describe the solution
Rewrite the runTest method similar to something in this experimental PR here. Only one test case has been rewritten here to be as similar to the original as possible. The output for the authorship files are not exactly the same as the one used currently and I have not been able to replicate it properly using only the CLI arguments.

Additional context

Immediate drawbacks

• The current implementation of RepoSense with this change will be unable to avoid multiple repeated cloning of the same few repositories for system test. One possible way to overcome this is to leave one test that performs clone and delete. For other tests, a copy of the remote repo is cloned locally and the tests are performed against the local copies instead.
• There are some parameters such as shallowCloning and findPreviousAuthors that still have to be passed to the runTest method as they directly affect the behaviour of the test case.
• The time generated for the report summary would be different from the expected JSON files. Some sort of bypass is needed to compare the summary JSON files while ignoring the time the report is generated.

[SkyBlaise99]

@chan-j-d, I think this can be done using InputBuilder. I realise that in generateReport(), these 2 are already handled by

if (shallowCloning) {
    inputBuilder = inputBuilder.addShallowCloning();
}
if (findPreviousAuthors) {
    inputBuilder = inputBuilder.addFindPreviousAuthors();
}

I believe the same can be done for the other variables. Namely TEST_REPORT_GENERATION_TIME and shouldFreshClone can be included into ArgsParser. Perhaps we can specify a Test Mode Flag in ArgsParser which is used for this senario.

A problem I found when attempting this is deleting of the directory in setUp() and tearDown(). This is because I used --output FT_TEMP_DIR, which results in the log files being written into the directory to delete. Hence the file is still being used, causing FileUtil to raise an exception. Temporary fix will be not to delete any of them in tearDown(). Another fix will be to add code into main to overwrite the required values.

[SkyBlaise99]

@chan-j-d can I try this issue? My idea is to add new flags into ArgsParser, like a -T test flag etc, and make main react differently, but minimally, when this flag is found.

[chan-j-d]

I'm not too sure I understand the approach you're taking. Drawback 3 has been somewhat addressed in this PR #1746. I think that PR was the main direction I had in mind where we construct tests in the form of CLI command arguments and just pass it into main instead of doing part of the parsing and config setting in the system test class.

You could share your implementation branch and I could take a look. If possible, I'd prefer not to add a flag just for testing to production code though you might have thought of something to make it work.

[SkyBlaise99]

A new flag is definitely required for shouldFreshClone though. Currently main uses DEFAULT_SHOULD_FRESH_CLONE = false, but some test cases requires the that to be true.

Also there is an extra line in main but not the original ConfigSystemTest. I tried to run with it but it causes all the test cases to fail. So I believe there is a need to add in a if (isTestMode) {...} into main for this small part. I'm not sure what the line does but it affects the output.

RepoSense/src/main/java/reposense/RepoSense.java

Line 108 in c3407f3

AuthorConfiguration.setHasAuthorConfigFile(false);

Here is my implementation branch.

In my case, I extended InputBuilder to construct the CLI input rather than what is done in #1746. I feel it is more readable this way. If you agree then perhaps I can make a small PR and use InputBuilder in LocalRepoSystemTest as well.

E.g.:

RepoSense/src/systemtest/java/reposense/ConfigSystemTest.java

Lines 45 to 53 in c3407f3

	@Test
	public void testSinceBeginningDateRange() {
	InputBuilder inputBuilder = initInputBuilder()
	.addSinceDate(SinceDateArgumentType.FIRST_COMMIT_DATE_SHORTHAND)
	.addUntilDate("2/3/2019");
	runTest(inputBuilder, false,
	"ConfigSystemTest/sinceBeginningDateRange/expected");
	}

I'll take a look at #1746 to see what I can take from it and add to Config System Test.

[SkyBlaise99]

@chan-j-d you can take a look at PR #1816 I just made.