Closed umerahmedkhan closed 3 years ago
The tests are failing but it doesn't look like it is a problem from your changes. I believe the name of the module is not set as the modules are not always configured in some tests.
We have two solutions for this problem:
The first solution is as easy as a single line. But I still prefer the second one. Actually, by definition, modules should not be usable before correct configuration. @juan-castrillon What do you think?
@umerahmedkhan For now just do the first solution.
@alyhasansakr You are right, the reason the tests are failing here is because we are passing "dummy" (not configured) modules in some tests, so they dont really have a logger (That explaines the NullPointerException
in the failing tests). Although adding a default name will not fix it, because the debug input (the logger) is created and added to the module just when setInputs()
is called.
I think the correct thing is configuring all the modules in the tests, however, doing it from a file, can be messy because the master is a singleton and therefore care needs to be taken to reset the instance and reconfigure every test. or the configuration of the master in one test can go to another completely unrelated test. That is done already in some tests like ScenarioEditor
and ScenarioDispatcher
. This allows us to really run the module in the test (needed in cases of scnearios, or REST for example)
Another alternative i can think of, would work if we only need to pass a module but not run it and is to create the raw data of the modules and then call the configure
method in the modules created in the test. That way we dont involve the master singleton. But i am not sure if we want to keep this practice of "dummy" modules
Regarding the definition, you are right, modules should not be allowed to be used before configuring them. However, i think we can only control that in calls to the start
method. Which in tests would only help, if alternative 1 above is implemented. Does that fall in the scope of this issue, because if not we can make a different one for that
@juan-castrillon Yes I see, by the way, why are we adding the debugInput within setInputs function? It should be done in configure function instead (implement it in PlatformModule and call super.configure when overridden).
For master configuration, it should reset all variables before a new configuration is done.
Alternative one makes more sense since this is the way it is configured normally (also we may have tests that use multiple modules at the same time).
The last point about modules without configuration. It should not be allowed to run any method in the module before running the configuration (throws an exception). This should be implemented in all modules. We need a separate issue for this.
@umerahmedkhan The chosen solution is to configure the master (and thus all modules) before each test, this way we are sure there is no difference between what we test and what we run.
An example is done in the function setUpMasterAndStartServer(). The configuration file you should use is the default one (src/main/resources/configuration.json). If it doesn't work for some cases, then we write them one by one.
Resolve #107 Added debug logs. The format of adding the debug log should be as follow: this.getName() + " - " + Msg A further improvement that we can make is that we can also add the function names in the debug logs. For that we can use different methods for example we can use Thread.currentThread().getStackTrace()[1].getMethodName() this returns the function name and we can place it after getName() and before message so that we can have a better traceability of the debug log messages.