Run configuration extra step #212
Conversation
|
Approved workflow run. |
There was a problem hiding this comment.
Hey, @just-rares! Thanks for your first MR!
I wrote somme comments! Would you mind adjusting the code based on them?
| if(!isValidRunConfiguration(runConfiguration)) { | ||
| // load default configuration | ||
| ctx.setRunConfiguration(new DefaultRunConfiguration(allClassesButTestingAndConfigOnes(ctx.getNewClassNames()))); | ||
| throw new RuntimeException("The run configuration has invalid weights. Please check the file."); |
There was a problem hiding this comment.
Let's throw a new exception, maybe AndyConfigurationException. Andy should stop in case the configuration is invalid, so in this sense, there's no need to set a default configuration!
Somewhere up we should catch this exception and generate an error output to the user. The message should clearly indicate this isn't the student's problem but the teacher's problem.
| * @param config The run configuration of the problem | ||
| * @return validity of the config weights represented as a boolean | ||
| */ | ||
| public static boolean isValidRunConfiguration(RunConfiguration config) { |
There was a problem hiding this comment.
This method would look better inside the RunConfiguration class itself. Look at it: 99% of it is interacting with data from that class, this is a sign that this method should be there.
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| public class RunConfigurationTests extends IntegrationTestBase { |
There was a problem hiding this comment.
These are unit tests, and they will look nicer once the rule is inside the RunConfiguration and you can simply call a config.isValid() rather than making a static method call to somewhere else
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class Configuration extends RunConfiguration { |
There was a problem hiding this comment.
I think we still need an integration test, with a wrong configuration, and then, we assert that the produced Result contains the error message. (See other integration tests in the integration folder)
There was a problem hiding this comment.
This should be the wrong configuration for one of the tests. Maybe I am understanding it wrong?
Here is the code for the test:
@Test
void checkExceptionHandling() {
Result result = run("NumberUtilsAddLibrary", "NumberUtilsAddAllTestsPass", "NumberUtilsAddConfigurationInvalid");
assertThat(result.getGenericFailure().getExceptionMessage().toString().contains("The run configuration has invalid weights. Please check the file.")).isTrue();
}Found in the test suite
There was a problem hiding this comment.
Ah ok, I missed the checkExceptionHandling. So, I'd split the unit tests from this integration test. Don't put them on the same file, as this isn't how we have been doing :)
|
I just noticed the |
|
I'm closing this one for now. We can always re-open once you are back to it! |
This pull request solves issue #173
Added a method in the
GetRunConfigurationStepthat checks the weights of the RunConfiguration and asserts whether they are valid with all the information available at the moment.Changes:
If the configuration is invalid, the default one is set and an exception is thrown.
I tests the functionality of this method in the
RunConfigurationTestsfile, where I used both unit tests to check the requirements of the method under test, some mocking to allow for disabling Pitest and Jacoco and one integration test with a library file with invalid weights to assert the error is indeed thrown and caught in theResultfile.Notes:
GradeCalculator. Some of the checks could not be done, such as the one with the grades. If it should be removed, please do let me know.Any suggestions are welcomed, I am not very confident about my implementation of this issue