Fix implementation of unstable tests

[frenzymadness]

A whole application can be unstable (like pipenv-test-app) not a specific test case (like test_application).

Cc: @zmiklank

[zmiklank]

Oh, from all the conversations and the naming of variables I understood, that it is the tests, which can be unstable, not apps, sorry. As I look again at your PR now, I see, that I must have overlooked that.

However, as my point of view on this topic changed now a lot, I begin to think, if it was not useful to implement the unstable apps and the unstable tests too.

This should be technically easy, as the apps are never named the same as the tests. It could look something like this:

if [[ " ${UNSTABLE_TESTS[*]} " =~ " ${test_case} " ]] ||  [[ " ${UNSTABLE_TESTS[*]} " =~ " ${app_name} " ]] then

However, I am not sure if this use case is needed or if it is important. Maybe this could be added just after some container would need that.

WDYT?

[frenzymadness]

My point of view might be biased because I know all the details only from the Python container but I think ignoring the results of unstable tests is needed only for applications, not for specific tests of that application.

If any external influence might impact a test, I want to mark the whole app as unstable because it does not matter which test I run on top of the application.

Correct me, if I'm wrong but this was my initial intention.

[phracek]

@frenzymadness But what about if UNSTABLE_TESTS is not defined: https://github.com/sclorg/s2i-nodejs-container/pull/337#issuecomment-1147171019

[zmiklank]

[test-all]

[zmiklank]

I have also proposed to let the user know, that the test result was ignored because of an unstable test. Retest: [test-all]

[phracek]

@zmiklank @frenzymadness Please rebase on the top of master.

[zmiklank]

@phracek, I have just rebased this branch on master.

[phracek]

[test]

[frenzymadness]

From my point of view, it LGTM. especially tag [FAILED][UNSTABLE-IGNORED]

I think the tag does not work as expected. See my inline comment.

[phracek]

This issue blogs https://github.com/sclorg/mariadb-container/pull/178#issuecomment-1148287702

[phracek]

From my point of view, it LGTM. especially tag [FAILED][UNSTABLE-IGNORED]

I think the tag does not work as expected. See my inline comment.

I see, it should be [PASSED][UNSTABLE-IGNORED]

[phracek]

@frenzymadness Sorry, I am lost with this issue. What should be the real outcome?

If the test is unstable what should be a tag in the results [FAILED] or [PASSED]. Definitely tests suit should not failed with 1 but 0

[frenzymadness]

@frenzymadness Sorry, I am lost with this issue. What should be the real outcome?

If the test is unstable what should be a tag in the results [FAILED] or [PASSED]. Definitely tests suit should not failed with 1 but 0

The new tag [UNSTABLE-IGNORED] should be there only if the result is really ignored but now it's there even if the result is not ignored and testsuite fails. I'm gonna fix that and send a commit here.

[frenzymadness]

I see that @zmiklank already changed that. Thank you. Let me test it.

[phracek]

[test]

[frenzymadness]

When I have pipenv-test-app (intentionally broken for the testing purposes) in the list of unstable tests and I do not specify IGNORE_UNSTABLE_TESTS, the result is:

…
 [FAILED] for 'pipenv-test-app' test_application (00:00:37)
 [FAILED] for 'pipenv-test-app' test_application_with_user (00:00:37)
 [FAILED] for 'pipenv-test-app' test_application_enable_init_wrapper (00:00:37)
…
Tests for quay.io/fedora/python3-39:0 failed.

And the exit code is 0. <-- this does not seem to be correct but it's probably fixed by https://github.com/sclorg/container-common-scripts/commit/b0bd592ffa3ee5da92296ea8c197aaba644c8468

When I specify IGNORE_UNSTABLE_TESTS=1 on the command line like IGNORE_UNSTABLE_TESTS=1 make test TARGET=fedora VERSIONS=3.9, the result is:

 [FAILED][UNSTABLE-IGNORED] for 'pipenv-test-app' test_application (00:00:37)
 [FAILED][UNSTABLE-IGNORED] for 'pipenv-test-app' test_application_with_user (00:00:38)
 [FAILED][UNSTABLE-IGNORED] for 'pipenv-test-app' test_application_enable_init_wrapper (00:00:38)
…
Tests for quay.io/fedora/python3-39:0 failed.

And the exit code is 0 which is fine.

I'm gonna rebase this branch to the top of the master branch and do another round of testing. Stay tuned.

[frenzymadness]

After a call with @phracek and @zmiklank we have realized that the exit code is a bug we have to fix and we're gonna do it here.

[frenzymadness]

I can confirm that it works now. I've tested all four combinations of the two new variables.

[frenzymadness]

@phracek wanna do the final check?

Also, don't forget to squash all the commits to one when merging.

[zmiklank]

@frenzymadness great. I have squashed the commits into two commits, as the testsuite result fix has a little common with unstable tests.

[zmiklank]

And @frenzymadness, thank you for the thorough review :)

[frenzymadness]

Thank you!