cucumber-rs / cucumber

Cucumber testing framework for Rust. Fully native, no external test runners or dependencies.
https://cucumber-rs.github.io/cucumber/main
Apache License 2.0
579 stars 70 forks source link

Book / Docs / Readme teach using Cucumber.run() - which does not make cargo test fail with non-zero exit code #334

Open Artalus opened 6 months ago

Artalus commented 6 months ago

84 brought a valid topic that using run() did not allow you to see if the Cucumber tests fail. The output of this ticket was new run_and_exit() method - which, unlike run(), will make your executable panic and exit with non-zero code.

However, the Book - together with Docs.rs and Readme - never mentions that .run() still has this behavior and that you can avoid it. The Book is still using the new method, but a-matter-of-fact'ly - some snippets just call .run_and_exit() instead of .run() without drawing any attention to it. This has led me to quite a surprise today when I noticed that my automation was not picking up some failures in Cucumber tests, since the executable using .run() was always exiting with 0, and so the cargo test command never failed technically πŸ™‚

My suggestion would be to

ilslv commented 6 months ago

To be fair, I don't quite understand what you are suggesting.

either simply mass-replace every use of .run( with .run_and_exit(;

As you mentioned earlier, book already uses .run_and_exit() for more complex setups.

or at least have a plaque right in Quickstart part of the Book, stating the difference between the two, if .run() is considered to be of some value.

Yes, Cucumber::run is still useful for example for internal tests. Instead of panicking, Writer can collect some info and then assertion can be made based on its content. About mentioning it in the quickstart guide, I don't think it's useful, as docs already have Panics section for every function of the public API that can panic.

Artalus commented 6 months ago

As you mentioned earlier, book already uses .run_and_exit() for more complex setups.

Yes, but it just... does it. Silently. I admit I skimmed over the later parts of the book instead of reading them carefully - but I did not even notice the change in the methods used.

Yes, Cucumber::run is still useful for example for internal tests. Instead of panicking, Writer can collect some info and then assertion can be made based on its content.

Thank you. That is a nice thing to have exposed in the API indeed! But:

About mentioning it in the quickstart guide, I don't think it's useful

I disagree. The quickstart makes it look like "to run the Cucumber tests and be all settled, you only call .run()". It does not mention that you need to do something with the result of .run() to make your test executable actually fail with your tests. It does not even indicate that .run() returns a meaningful result you can use later on. I haven't looked into docs until I encountered my problem but I was sure it returns () - otherwise the book would mention it?..

To be fair, I don't quite understand what you are suggesting.

I am suggesting that the "default" way to run the tests - taught by the book in its very first "how to" chapter - should involve exiting with non-zero code on test failure. Especially if they are intended to be a part of cargo test suite. Otherwise you get a nice red output from cargo test, but for example your CI status would still be green.


Interesting enough, my results seem to differ from the book. In the first picture of a failing test there are both thread 'main' panicked and error: test failed, to rerun pas --test example lines. Yet if I run the code from quickstart my tests seem to pass from cargo perspective:

Artalus@ArtPC $ cargo test --test example
     Running tests\example.rs (target\debug\deps\example-ccc7c40af0e2c213.exe)
Feature: Animal feature
  Scenario: If we feed a hungry cat it will no longer be hungry
   βœ”  Given a hungry cat
   βœ”  When I feed the cat
   ✘  Then the cat is not hungry
      Step failed:
      Defined: ?\D:\rust\tests\features\animal.feature:6:5   
      Matched: tests\example.rs:36:1
      Step panicked. Captured output: assertion failed: world.cat.hungry
[Summary]
1 feature
1 scenario (1 failed)
3 steps (2 passed, 1 failed)

Artalus@ArtPC $ echo $?
0