app_stack: Fix macro regression.

[InterLinked1]

Fixes a regression introduced in commit
617dad4,
where executing GoSub subroutines inside
macros will cause issues. This undoes the
check added in that commit for macros to
ensure the same behavior as before.

Resolves: #253

[InterLinked1]

cherry-pick-to: 18

[seanbright]

Tests need to be written to exercise the current behavior before this can be merged.

[InterLinked1]

Tests need to be written to exercise the current behavior before this can be merged.

Good idea, but how would this work exactly? I don't think the Asterisk test suite has any way to check CLI output or channel internals... does it? Maybe a better question, what aspect specifically should be tested?

[jcolp]

It caused an actual failure in dialplan execution for @creslin287 which is testable through the testsuite, so if that situation can be identified then a test can be written for it.

[InterLinked1]

It caused an actual failure in dialplan execution for @creslin287 which is testable through the testsuite, so if that situation can be identified then a test can be written for it.

Yes, but tests cannot be written for master and 21, since macro support has been removed entirely from there. So the tests would only apply to 20 and 18 - or am I missing something here?

[jcolp]

An effort should be put forth to include test coverage for 21 and master. If not possible then an explanation of why not and what would be required to actually do testing would be needed. The functionality that should be tested is what change to behavior it has caused.

[jcolp]

This is something that we've all become lax on, to the detriment of the project. We should not brush off writing tests if they're hard. We should put forth any and all effort we can to include test coverage.

[InterLinked1]

An effort should be put forth to include test coverage for 21 and master. If not possible then an explanation of why not and what would be required to actually do testing would be needed. The functionality that should be tested is what change to behavior it has caused.
This is something that we've all become lax on, to the detriment of the project. We should not brush off writing tests if they're hard. We should put forth any and all effort we can to include test coverage.

Adding regression tests for this will be easy. Testing the functionality itself is hard because there's no good way I can think of to do so.

I see 3 possible options for that:

1. Test manually as before - this is a very specific change so if it works, it works. But not ideal, as it won't prevent it from breaking in the future.
2. Test by examining CLI output. I don't think there is a mechanism to do this though, and I am not sure adding one is worth it. Text parsing is also... fragile.
3. Add it as a unit test instead of a test suite test. It will be a bit hacky, but we should be able to make a dummy channel execute some applications and then poke around in the internals to make sure we find what we expect to find. I'm not sure how a unit test can elegantly execute dialplan though. Might have to add some extensions using the APIs when starting the test and remove them afterwards.

Again, I'm referring to the functionality test here, not the regression tests which can be done in the test suite easily.

Option 3 seems the best to me, even though it's going to be messy. Or can you think of a way this could be done in test suite?

[jcolp]

Channels are channels, executing dialplan is executing dialplan. Doing it as a unit test is likely possible.

I don't know the full requirements or constraints so I can't say otherwise.