Broken step results in a successful run

[Emptyfruit]

Describe the bug (Probably related to https://github.com/ozontech/allure-go/issues/4)

A single test with a broken step results in a passed test.

The test

func Test_AllureExample(t *testing.T) {
    runner.Run(t, "Allure demo", func(t provider.T) {
        t.WithNewStep("First step", func(sCtx provider.StepCtx) {
            sCtx.Logf("Demo break")
            sCtx.Broken()
        })
    })
}

The report (exluding labels):

{
    "name": "Allure demo",
    "fullName": "Test_AllureExample/Allure_demo",
    "status": "passed",
    "statusDetails": {
        "message": "",
        "trace": ""
    },
    "start": 1669278069758,
    "stop": 1669278069758,
    "uuid": "f34c46a8-6bd0-11ed-bd95-a45e60d5053f",
    "historyId": "baa17e747dc3baf09c0ad0b7a00c6251",
    "testCaseId": "3948cbd85816212b9f92d81b0b03b207",
    "labels": [
    ],
    "steps": [
        {
            "name": "First step",
            "status": "broken",
            "start": 1669278069758,
            "stop": 1669278069758
        }
    ]
}

Note that step has a proper broken status, but the test has a passed status.

A clear and concise description of what the bug is.

To Reproduce Steps to reproduce the behavior:

1. Create a single test.
2. Define one step in the test.
3. use broken() in this step's function.
4. Check report.

Expected behavior The test is marked as Broken in the report.

Additional context I have some questions regarding this issue:

1. Is it a bug or is it intended? If intended, how can i apple Broken status to a test?
2. What should be done either way with provider.T of my test after setting some step to broken()? There is no BreakNow() fn, so i just have to end the test execution naturally after a broken step?
3. Which go test status (skipped / failed) will be applied to the test, if it s properly set as Broken by the library?

[koodeex]

Hello, @Emptyfruit ! Thank you for your feedback.

Q: Is it a bug or is it intended? If intended, how can i apple Broken status to a test? A: For now its intended - broken status for routine panic only. All other cases should be handled with assert/require/fail/failnow methods. The reason of this is broken status semantics. Broken means break of test code, not test logic. So you can broke your step, but you should not break with it your test. Can you please describe case that you tries to solve with Broken() method?

Q: What should be done either way with provider.T of my test after setting some step to broken()? There is no BreakNow() fn, so i just have to end the test execution naturally after a broken step? A: Expected behavior is use t.FailNow() for shutting down in unexpected situations

Q: Which go test status (skipped / failed) will be applied to the test, if it s properly set as Broken by the library? A: we use failed cause skipped has different behaviour and semantics (this is the reason why you can't just call Broken() to set broken status.

[Emptyfruit]

Thanks for the quick and detailed replies.

Our use-case is pretty complex: we have a set of test cases (from go project's perspective it is dynamic subtests). All cases are based on a custom data format and stored remotely, so getting an actual test case is a process of several steps (finding, parsing, etc.) and each can fail for different reasons. So if the "preparation" of a test case fail, we consider it broken. And only if it is prepared and we have and actual case to test, it can actually fail or succeed. It seems like a legit use of broken to me.

I believe that if Allure report has this status for a test itself (no only steps) and even statistics for it, there should be an option to use it. Btw, i tried dailymotion/allure-go and it has Break() and BreakNow() and it works as i'd imagine. (though, the lib itself has a very limited reporting functionality).

Also i'd note that currently this aspect feels a bit "undecided" when using you library. By default if i just break a step the test will be considered successful, which a bit counter-intuitive for me. imho, i cannot call a test with a broken step "successful". I'd prefer to notice something is wrong, because it means something is probably left untested. But i'm new to qa engineering, tbh.

[koodeex]

@Emptyfruit , if you have some tests that are depends from each other best solution in this case is to use asserts
Go allows you something like this:

func (s *MySuite) TestMyTest(t provider.T)  {
     var (
           err error

           firstSubtestResult SomeInterface
           secondSubtestResult SomeInterface
           thirdSubtestResult SomeInterface
     )

     t.Run("First subtest", func(t provider.T) {
          firstSubtestResult, err = s.SomeCli.SomeApiCall(...)
          t.Require().NoError(err)
     }
     t.Require().NotNil(firstSubtestResult)

     t.Run("Second subtest", func(t provider.T) {
          secondSubtestResult, err = s.SomeCli.SomeApiCall(...)
          t.Require().NoError(err)
     }
     t.Require().NotNil(secondSubtestResult)

     t.Run("Third subtest", func(t provider.T) {
          thirdSubtestResult, err = s.SomeCli.SomeApiCall(...)
          t.Require().NoError(err)
     }
     t.Require().NotNil(thirdSubtestResult)
}

In your case it can be best solution. Each part of your big e2e test should be asserted. Cause failing of subtest is defect, not issue with tests.
I agree that Broken() behavior in StepCtx interface is unclean, but the reason, why it works in that way is simple - if you need panic inside step, but don't wan't to fail test (panic expected) it will be Broken() called inside the library to set status to the step, but test still will be green.

Also "testing" library of golang not provides broken status to the tests (only success/fail) so broken can be confusing in many cases. Native way to end test immidiately on reaching some condition is FailNow() for Go

[koodeex]

It also ok to use closures in your tests. It kind of native way to exchange data between lambda calls inside the go

[koodeex]

Native way to end test immidiately on reaching some condition is FailNow() for Go

BTW, the reason of this is specific goway to work with exceptions (returning errors as function result and panic handling). You don't need to throw exceptions like in Java or C# - cause panic is really unexpected behavour (like NPE, or Stackoverlow). So all other situations that provide errors is actually expected.

if you have errors in go - it means that test failed

If you have unhandled panics - it means that test broken

Please, leave feedback if you have any other questions. I will be enjoyed to help you in building tests with allure-go :)

P.S. Step status control in StepCtx interface has been added to provide more flexability to integrations (like cute) and some custom step handlers in users tests.

[Emptyfruit]

Thank your for interesting advices. I'll try to clarify our situation a bit. I cannot have suits the way you implement them in go, because all my cases are dynamic. So i have only one TestAllCases(t *testing.T) test in my go project. So each dynamic case will be a separate suite in my report (which is totally fine by me).

I'd prefer not to treat the "setup" steps as part of the test. It is just a matter of convenience. If i open a report and see 100 failed tests, i have no way of knowing which failure is an indeed a failed test of functionality and which failure is just a misconfigured test case (which lives inside another project and is managed by another team). So i'd like to separate valid cases that failed (have to fix product) and invalid cases that say nothing about the product's functionality (have to fix test case or test case repo).

As i understand, i could define one Suite and treat each dynamic test case as a subtest of this suite. That way i'd even get my "broken" functionality, since setup-steps will become substeps and breaking will propogate to the level of my case. So in my report i will have 1 suite with hundrends of steps (cases) inside. But i think that having all tests as a steps of one suite is kind of a workaround and i'll loose most allure functionality in the end, such as history, tags, stats, timing, etc.

I understand your position on panics and error handling. But still, i think that providing an api to set test's status to Broken (even if i have to explicitly set it) will just add flexibility. In the end, it is just a matter of interpretation of a report. As i understand now, there is no way whatsoever to get a suite with status Broken in your report. So i see reasons not to use it for some implicit behaviour, but no reasons to restrict setting it manually.

So in the end, i'd be happy just to have a Break() or BreakNow() method, which would do the same as Fail() and Failnow() but set the suite's status to Broken, which i would use explicitly. I don't see much harm in it, but see some flexibility.