Integrate fake logger with test configuration.

[albu-diku]

Early in the introduction of the supporting libraries, the notion of a FakeLogger was introduced. This is an object that is usable as a substitute .logger property on the MiG configuration and will capture anything output for each channel for the purposes of being able to assert against later.

Almost all codepaths which handle internal error conditions send messages to the error channel. Thus, we can use the presence of messages in the error channel (or lack thereof) as a proxy for whether anything internal went wrong. The wiring to assert no errors were recorded has been there since the initial introduction of FakeLogger, but in order for it to kick in we must be using the instance for the duration of each test case.

The missing piece was ensuring that when we request the "testconfig" via a _provide_configuration() call that we assign the test logger. This commit does that and hence integrates them correctly. Doing so catches a number of cases where we either fail to account for error messages or had conditions causing unexpected errors to be generated - these are repaired here as well.

[albu-diku]

@jonasbardino This is the branch I referenced earlier - battening down this particular hatch is surfacing some otherwise silent errors, and I could use second eyes in assessing whether behavioural issues are being highlighted.

There are some issues with a common cause being highlighted - my suggestion is using ./envhelp/lpython -m unittest discover ./tests -f within a checkout to run until the first failure and stop.

The first one I'm seeing that looks decidedly suspect is in grid_events, where banning error logs is catching the following:

RuntimeError: errors reported to logger:
(31894) text formating failed: id() takes exactly one argument (4 given)
raw output is: ...

[jonasbardino]

Looks fine but I'll take a look at the tests as suggested before approving.

[albu-diku]

@jonasbardino are you sure about that diagnoses wrt the grid events tests? I don’t know the code very well, but reading it I did see it appears to be calling id with multiple arguments in the text formatting case.

I’ll add the fabrication empty settings files here and see what the situation is, but I don’t immediately see how text formatting would be changed. Could I ask you to read over that again?

[jonasbardino]

@jonasbardino are you sure about that diagnoses wrt the grid events tests? I don’t know the code very well, but reading it I did see it appears to be calling id with multiple arguments in the text formatting case.

I’ll add the fabrication empty settings files here and see what the situation is, but I don’t immediately see how text formatting would be changed. Could I ask you to read over that again?

I'll look into it. I already identified and fixed some issues in test_mig_shared_vgrid.py with missing mig_system_files dir and the use of vgrid_add_X over vgrid_set_X causing log noise and therefore failing with extended log monitoring.

[albu-diku]

@jonasbardino are you sure about that diagnoses wrt the grid events tests? I don’t know the code very well, but reading it I did see it appears to be calling id with multiple arguments in the text formatting case.
I’ll add the fabrication empty settings files here and see what the situation is, but I don’t immediately see how text formatting would be changed. Could I ask you to read over that again?

I'll look into it. I already identified and fixed some issues in test_mig_shared_vgrid.py with missing mig_system_files dir and the use of vgrid_add_X over vgrid_set_X causing log noise and therefore failing with extended log monitoring.

Great and thanks. Can see bits and pieces trickling in (though haven’t had a chance to look deeply at them yet for transparency) which is great - think this tightening will serve us well going forward!

I’ll obviously contribute to straightening out the machinery and patching things wherever I can, so let me know what I can handle as part of the joint exercise :)

[jonasbardino]

@jonasbardino are you sure about that diagnoses wrt the grid events tests? I don’t know the code very well, but reading it I did see it appears to be calling id with multiple arguments in the text formatting case.
I’ll add the fabrication empty settings files here and see what the situation is, but I don’t immediately see how text formatting would be changed. Could I ask you to read over that again?

I'll look into it. I already identified and fixed some issues in test_mig_shared_vgrid.py with missing mig_system_files dir and the use of vgrid_add_X over vgrid_set_X causing log noise and therefore failing with extended log monitoring.

Great and thanks. Can see bits and pieces trickling in (though haven’t had a chance to look deeply at them yet for transparency) which is great - think this tightening will serve us well going forward!

I’ll obviously contribute to straightening out the machinery and patching things wherever I can, so let me know what I can handle as part of the joint exercise :)

AFAICT the failing id(X) calls come from the events command lookup failing as expected when a bogus command is provided. Then it defaults to the id function rather than a matching actual backend main function. It probably needs some more work to iron out or adjust for a clean exit. I'll continue later.

[albu-diku]

I've added a detailed description that explains the context and the changes being made here.

[albu-diku]

@jonasbardino are you sure about that diagnoses wrt the grid events tests? I don’t know the code very well, but reading it I did see it appears to be calling id with multiple arguments in the text formatting case.
I’ll add the fabrication empty settings files here and see what the situation is, but I don’t immediately see how text formatting would be changed. Could I ask you to read over that again?

I'll look into it. I already identified and fixed some issues in test_mig_shared_vgrid.py with missing mig_system_files dir and the use of vgrid_add_X over vgrid_set_X causing log noise and therefore failing with extended log monitoring.

Great and thanks. Can see bits and pieces trickling in (though haven’t had a chance to look deeply at them yet for transparency) which is great - think this tightening will serve us well going forward!
I’ll obviously contribute to straightening out the machinery and patching things wherever I can, so let me know what I can handle as part of the joint exercise :)

AFAICT the failing id(X) calls come from the events command lookup failing as expected when a bogus command is provided. Then it defaults to the id function rather than a matching actual backend main function. It probably needs some more work to iron out or adjust for a clean exit. I'll continue later.

Cool - do yo want me to do something there or shall I leave that to you now you're looking at it?

[jonasbardino]

@jonasbardino are you sure about that diagnoses wrt the grid events tests? I don’t know the code very well, but reading it I did see it appears to be calling id with multiple arguments in the text formatting case.
I’ll add the fabrication empty settings files here and see what the situation is, but I don’t immediately see how text formatting would be changed. Could I ask you to read over that again?

I'll look into it. I already identified and fixed some issues in test_mig_shared_vgrid.py with missing mig_system_files dir and the use of vgrid_add_X over vgrid_set_X causing log noise and therefore failing with extended log monitoring.

Great and thanks. Can see bits and pieces trickling in (though haven’t had a chance to look deeply at them yet for transparency) which is great - think this tightening will serve us well going forward!
I’ll obviously contribute to straightening out the machinery and patching things wherever I can, so let me know what I can handle as part of the joint exercise :)

AFAICT the failing id(X) calls come from the events command lookup failing as expected when a bogus command is provided. Then it defaults to the id function rather than a matching actual backend main function. It probably needs some more work to iron out or adjust for a clean exit. I'll continue later.

Cool - do yo want me to do something there or shall I leave that to you now you're looking at it?

Alright, it turned out to be only slightly similar cases of ending up with id for txt_output in valid tests. I believe I've fixed the underlying bug with #479 and we should merge that in here as well to resolve those error logs.

[jonasbardino]

I think we finally covered all the silently failing issues this change revealed and CI succeeds again.

Please note that in order to get a few last failing accountreq and janitor test cases running I had to inject forgive_errors for the time being, because they fail on send_email in notification. While we have already discussed solutions to that elsewhere it can't be easily overridden, yet. I've included TODOs about those workarounds and the follow-up tag here.

[albu-diku]

I think we finally covered all the silently failing issues this change revealed and CI succeeds again.

Please note that in order to get a few last failing accountreq and janitor test cases running I had to inject forgive_errors for the time being, because they fail on send_email in notification. While we have already discussed solutions to that elsewhere it can't be easily overridden, yet. I've included TODOs about those workarounds and the follow-up tag here.

#467 is open to address this and was updated to line up with he outcomes of the discussion referenced above.