Consolidate SKIP_E2E_SUBS and E2E_SUBS_ONLY

[algojack]

Summary

Replaced SKIP_E2E_SUBS var and E2E_SUBS_ONLY var with E2E_SUBS var that has values of "ONLY" and "SKIP" as well as not set at all. We might need to add another value to this var in the future for another option.

Test Plan

Check if tests work correctly still.

[codecov-commenter]

Codecov Report

Merging #2705 (63a0c09) into master (7e84d77) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2705      +/-   ##
==========================================
+ Coverage   47.08%   47.09%   +0.01%     
==========================================
  Files         349      349              
  Lines       55887    55887              
==========================================
+ Hits        26312    26319       +7     
+ Misses      26626    26621       -5     
+ Partials     2949     2947       -2     

Impacted Files	Coverage Δ
ledger/blockqueue.go	83.90% <0.00%> (-1.15%)	⬇️
cmd/tealdbg/debugger.go	72.86% <0.00%> (-1.01%)	⬇️
network/wsNetwork.go	60.73% <0.00%> (-0.19%)	⬇️
ledger/acctupdates.go	62.30% <0.00%> (+0.41%)	⬆️
network/wsPeer.go	74.37% <0.00%> (+2.22%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e84d77...63a0c09. Read the comment docs.

[algobarb]

I think this looks ok with the new change if we want to leave the option of adding another option in the future.

[algojack]

Talked to Will, will be refactoring to use switch statement.

[winder]

It looks like it would work, but has the same mixed skip/only pattern so in general, the design seems strange to me. There exists this undefined "other stuff" that doesn't need to be so mysterious.

I think selecting which tests to run would be easy to understand, and flexible for future subdivisions (like when we re-enable expect tests):

if [ -z $E2E_TEST_FILTER || $E2E_TEST_FILTER="SCRIPTS" ]; then
    run_e2e_subs
fi
if [ -z $E2E_TEST_FILTER || $E2E_TEST_FILTER="GO" ]; then
    run_go_tests
fi
if [ -z $E2E_TEST_FILTER || $E2E_TEST_FILTER="EXPECT" ]; then
    run_go_tests --expect
fi

A switch/case could also be nice if wrapping everything in if's is too ugly

[algojack]

@winder Changed to switch case and combined into one variable as we discussed. Refactoring everything in that file into it's own if statements and adding variables where it's used might be a bit outside of the scope of this ticket. So we should file another ticket if you want to refactor this file. What do you think?

[winder]

This looks more complex with no benefit. My main concern was that we used both enable and skip so this has the same configuration complexity and extra code complexity. Why not use a simple if block that supports future cases with a direct configuration and simple code?

Either way I'll defer to the DevOps team. Whoever re-enables the expect tests will need to revisit this code.

[algojack]

that's what i thought too, that's why i was confused when you said you wanted switch statement. I don't think we understand what you want here @winder

[algojack]

putting this ticket in blocked/icebox until we have clear description and delivierables.

[algojack]

@winder how about this? If this is not it, we should definitely have another call once you are back. Apologies if I misunderstood.

[algobarb]

It looks like it would work, but has the same mixed skip/only pattern so in general, the design seems strange to me. There exists this undefined "other stuff" that doesn't need to be so mysterious.

I think selecting which tests to run would be easy to understand, and flexible for future subdivisions (like when we re-enable expect tests):

if [ -z $E2E_TEST_FILTER || $E2E_TEST_FILTER="SCRIPTS" ]; then
    run_e2e_subs
fi
if [ -z $E2E_TEST_FILTER || $E2E_TEST_FILTER="GO" ]; then
    run_go_tests
fi
if [ -z $E2E_TEST_FILTER || $E2E_TEST_FILTER="EXPECT" ]; then
    run_go_tests --expect
fi

A switch/case could also be nice if wrapping everything in if's is too ugly

Why is the if statement checking for -z $E2E_TEST_FILTER across all of these?
Do we expect to run all tests when -z $E2E_TEST_FILTER?

E2E_TEST_FILTER just seems a bit odd to me. Maybe it could check for $E2E_TEST = "ALL" so that it is more explicit when to run all of the tests and when it will only run only "SCRIPTS" and "GO" e2e tests?

[algojack]

Why is the if statement checking for -z $E2E_TEST_FILTER across all of these?
Do we expect to run all tests when -z $E2E_TEST_FILTER?

Yes, that's the default right now. We have places that run this without any flags.

E2E_TEST_FILTER just seems a bit odd to me. Maybe it could check for $E2E_TEST = "ALL" so that it is more explicit when to run all of the tests and when it will only run only "SCRIPTS" and "GO" e2e tests?

I'm not sure that's where Will wants to go with this, so far as I understand it we want the default (no flags) to be defined to run these (vs not running)

[algobarb]

Why is the if statement checking for -z $E2E_TEST_FILTER across all of these?
Do we expect to run all tests when -z $E2E_TEST_FILTER?

Yes, that's the default right now. We have places that run this without any flags.

E2E_TEST_FILTER just seems a bit odd to me. Maybe it could check for $E2E_TEST = "ALL" so that it is more explicit when to run all of the tests and when it will only run only "SCRIPTS" and "GO" e2e tests?

I'm not sure that's where Will wants to go with this, so far as I understand it we want the default (no flags) to be defined to run these (vs not running)

In that case, I guess that makes sense.