Mocked CAF::FileWriter attempts to create directories

[quattor]

Mocked CAF::FileWriter attempts to create directories

[stdweird]

quattor/CAF#202 should fix this. we should retest once that PR is tested and merged.

[jouvin]

quattor/CAF#202 may fix it but I checked the Test/Quattor.pm test and I don't understand why it fails as LC_Check is supposed to be mocked... Something strange to me: Test/Quattor.pm has an exported function make_directory but there is also a make_directory in simple_caf used by tests... Logic not clear to me...

[jouvin]

I manage to find the culprit... This is the mocked CAF::FileWriter::close(). It calls the original CAF::FileWriter::close() which seems a little bit odd as there is no point to mock it if we call it (well it is not completely true as the mocked version updates the %desired_file_contentshash)... Removing this call, I have been able to check that maven tools build-scripts can be built successfully and that it does fix the problem in aii-pxelinux. But it remove of the diff handling and the reporting about the differences: I don't know if this may be a problem in other contexts. Another approach could be not to execute the LC::Check::file call if noaction is set (which is the case as it is set by the mocked constructor).

@stdweird any opinion?

[jouvin]

I had a look as the second solution (set noaction option before calling the original close() and not calling LC::Check::file in this case). It looks to me the right way to do it but in this case we also need to force noAction in the mocked context which is not currently the case conversely to what I said... Comments welcome!

[stdweird]

how can i repoduce this issue? and have you tried the new filewriter? i have never had an issue with the original code; but we probably run NoAction=1 everywhere where we shouldn't.
best way to deal with this is proabably shipping a dummy LC or Atomic module with maven-tools, similar to the CAF uses in its unittests and also the components.

[jouvin]

@stdweird I found this issue after your remark that we were abusing NoAction=1 in the unit tests and that it was not necessary now that CAF was mocked in maven tools... You can easily reproduce the problem by running write_pxelinux_config.t in UEFI PR after removing NoAction=1.

After thinking more at this problem, I think that what is wrong is to execute the original method in the mocked one. I've the feeling that the right approach is what I did with links: have a mocked version that is implementing the feature of the original one (like reporting diffs for example).

An alternative could be to set noaction option (at least by default) before calling the original close() but I don't know if if may break some tests in other Quattor components...

[stdweird]

ok, i see.
yes, Noaction is not used properly in most unittests and this issue would indeed prevent people from doing so.
however, i'm not convinced we should reinplement the CAF code here, shipping a dummy LC or Atomic module will do the same trick with a lot less work.

but there are also cases where the mocking cannot be done, eg during the test profile CCM part. that needs fully functional FileWriter. it's also not a big issue as long as all compilation is triggered via the import hack.

[jouvin]

@stdweird your CCM use case is a reason I was proposing to set noaction option of the FW instance to 1 by default in the constructor or in close(). This would allow to define CAF::Object:NoAction=0 when this is needed (and in this case noaction option will be set to 0 too).

[jouvin]

I just pushed a commit implement the noaction idea to make it more clear! I tested that it does fix the aii-pxelinux issue when NoAction=1 is removed from unit tests and that CAF unit tests work properly too. I could not verify CCM as I fail to run successfully the tests on the master without the modified build tools... @stdweird as you have a cleaner dev environment than me, may be you could validate the impact and if it is enough to define $CAF::Object::NoAction=0 to fix problems if any?

[jrha]

I believe this was closed by #160.