Add documentation for `{fail,Reason}` to ct test case return values #7869

Maria-12648430 · 2023-11-15T16:29:16Z

In the course of #7824 it was discovered that when a test case returns {fail, Reason} the test case is considered failed. This return value is explicitly used by the property testing facility of ct to indicate failing properties.

However, the docs for ct_suite do not mention it, and as it stands, this passage in the ct_suite docs reading "If the function returns anything else [except {skip, Reason}], the test case is considered successful." and this passage in the User Guide reading "If the test case function crashes or exits purposely, it is considered failed. If it returns a value (no matter what value), it is considered successful. An exception to this rule is the return value {skip,Reason}." contradict reality.

IngelaAndin · 2023-11-28T08:57:18Z

lib/common_test/doc/src/write_test_chapter.xml

-    considered successful. An exception to this rule is the return value
+    <p>If the test case function crashes, exits purposely or returns <c>{fail,Reason}</c>,
+    it is considered <em>failed</em>. If it returns any other value (no matter what value),
+    it is considered successful. An exception to this rule is the return value


As we are improving this I was thinking maybe we should rephrase to

"If {skip, Reason} is returned the test case will be skipped and the Reason logged, otherwise if it returns any other value (no matter what value), the test case is considered successful." I think it sounds better!

I think return value of a testcase is an important part of this section and possibly being common reason people visiting this chapter. IMHO having single paragraph for describing all possibilities is not optimal here and not easy to find when not reading sequentially.

Maybe we could use some form of enumeration to make it more visible what return value could be and how it will be interpreted (similarly to how it is explained in previous init/end_per_testcase section)?

IngelaAndin

Suggested rephrasing, see code comment!

u3s · 2023-11-28T10:02:09Z

lib/common_test/doc/src/write_test_chapter.xml

-    considered successful. An exception to this rule is the return value
+    <p>If the test case function crashes, exits purposely or returns <c>{fail,Reason}</c>,
+    it is considered <em>failed</em>. If it returns any other value (no matter what value),
+    it is considered successful. An exception to this rule is the return value


I think return value of a testcase is an important part of this section and possibly being common reason people visiting this chapter. IMHO having single paragraph for describing all possibilities is not optimal here and not easy to find when not reading sequentially.

Maybe we could use some form of enumeration to make it more visible what return value could be and how it will be interpreted (similarly to how it is explained in previous init/end_per_testcase section)?

u3s · 2023-11-28T10:18:52Z

lib/common_test/doc/src/ct_suite.xml

@@ -604,6 +604,9 @@
 	<p>To print some information in field <c>Comment</c> on the HTML
 	  result page, return <c>{comment, Comment}</c>.</p>

+        <p>If the function returns <c>{fail, Reason}</c>, the test case
+          is considered as failed.</p>
+
 	<p>If the function returns anything else, the test case is


While reviewing this part, maybe we could improve clarity here as well ... I hope :-)
It think this is not as clear about {comment, Comment} as it should/could be. What do you think?

{comment, Comment} paragraph is not explicitly suggesting "The test case is considered successful, but with a comment."

"If the function returns anything else, the test case is considered successful.". When people learn and can't absorb all information or just skim docs, I'm afraid they might draw a conclusion that a testcase returning {comment, Comment} will not be considered successful.

Maybe I'm picky here, but I had students asking those questions :-) It would be great if I could redirect them to clear docs instead of explaining it myself ;-).

Maria-12648430 · 2023-11-29T07:05:49Z

I think @u3s's suggestions make sense, if @IngelaAndin agrees I will break up the single paragraph and augment the docs for more clarity?

IngelaAndin · 2023-11-29T07:12:30Z

@Maria-12648430 sure go ahead. I think my suggestion was a small improvement but @u3s idea will end up being a bigger improvement.

github-actions · 2023-11-29T12:44:47Z

CT Test Results

    2 files   58 suites 1h 24m 20s ⏱️
450 tests 438 ✔️ 12 💤 0 ❌
485 runs 470 ✔️ 15 💤 0 ❌

Results for commit 409df69.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

Maria-12648430 · 2023-11-29T12:45:35Z

I pushed a first draft for the ct_suite docs, for review. If you agree with this, I will do the respective paragraph in the user guide along the same lines.

IngelaAndin · 2023-11-29T14:46:01Z

lib/common_test/doc/src/ct_suite.xml

+
+	<p>If the function returns any other term, the test case is considered
+	  successful. It is common practice to explicitly return <c>ok</c> from
+	  successful test cases.</p>


I am not sure we want to spread that practice, even though it is quite used it can be very useful not to return ok as you can then see in the log what was the result of the last action in the test case.

Ok, agreed 👍

Removed the sentence in the last commit.

IngelaAndin · 2023-11-29T14:51:54Z

lib/common_test/doc/src/ct_suite.xml

+	  successful test cases.</p>
+
+	<p>If the test case function crashes or exits purposely, it is considered
+	  failed.</p>


Exits purposely is a lite vague! When the test case code ends the test case process will exit purposely with the reason normal and it will be successful, if it exits not normal either by crashing or by calling ct:fail the test case will fail. (Additionally it will also fail if it returns {fail, Reason} as we now know)

That's the phrase used in the user guide, I just kinda copied that. But I agree 😉

Removed "or exits purposely" in last commit.

Well our documentation is far from perfect ;-) I think we should strive for continuous improvement :)

Oh, I think it is pretty good actually :) But we can always improve.

Well every ting is relative, and of course many parts are really good but I believe there is still room for improvement in some places more than others.

Of course :) Generally, the reference is pretty good I think. "This is what it expects, this is what it does, this is what you get", nice and concise. The User Guide is another thing, and not an easy one, as much of what is (not) important depends on the reader, so what is too vague for one reader may be too pedantic or long-winded to another. I always struggle when I have to write something in there =^^=

u3s · 2023-11-29T15:15:01Z

I pushed a first draft for the ct_suite docs, for review. If you agree with this, I will do the respective paragraph in the user guide along the same lines.

I like proposed format and also agree with comments made by Ingela.

Maria-12648430 · 2023-11-29T16:58:47Z

Last commit adapts the detailed return value documentation for the User Guide.

u3s

Thanks for making a more general improvement!

IngelaAndin · 2023-11-30T15:16:17Z

I like what you wrote but not that it will be duplicated in the user guide. Could we not let the user guide link to man page.
Write something like. The test case result can be customized in several ways see manual for details.

Maria-12648430 · 2023-11-30T16:06:02Z

Could we not let the user guide link to man page.

Sure, good idea :)

Maria-12648430 · 2023-11-30T16:40:35Z

There, is that better? 😺

IngelaAndin · 2023-12-04T07:53:03Z

Thanks for PR

Maria-12648430 mentioned this pull request Nov 15, 2023

Fix ct_property_test result handling for failing properties #7824

Merged

u3s added team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI labels Nov 16, 2023

u3s self-assigned this Nov 16, 2023

u3s added the priority:low label Nov 16, 2023

IngelaAndin reviewed Nov 28, 2023

View reviewed changes

IngelaAndin self-requested a review November 28, 2023 08:57

IngelaAndin requested changes Nov 28, 2023

View reviewed changes

u3s reviewed Nov 28, 2023

View reviewed changes

IngelaAndin added the waiting waiting for changes/input from author label Nov 28, 2023

Add documentation for {fail,Reason} to ct test case return values

6216c9f

Maria-12648430 force-pushed the fix_ct_testcase_return_docs branch from a3e628c to 6216c9f Compare November 29, 2023 12:43

IngelaAndin reviewed Nov 29, 2023

View reviewed changes

Maria-12648430 added 2 commits November 29, 2023 17:55

Add suggested changes to ct_suite docs

d02ca39

Improve test case return value documentation

1d78256

Maria-12648430 force-pushed the fix_ct_testcase_return_docs branch from 510351a to 1d78256 Compare November 29, 2023 16:57

u3s approved these changes Nov 30, 2023

View reviewed changes

Maria-12648430 requested a review from IngelaAndin November 30, 2023 13:37

Replace repeated test case return value docs with a link to the manual

409df69

IngelaAndin approved these changes Dec 1, 2023

View reviewed changes

IngelaAndin removed the waiting waiting for changes/input from author label Dec 1, 2023

IngelaAndin merged commit c5b8188 into erlang:master Dec 4, 2023
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add documentation for `{fail,Reason}` to ct test case return values #7869

Add documentation for `{fail,Reason}` to ct test case return values #7869

Maria-12648430 commented Nov 15, 2023

IngelaAndin Nov 28, 2023

u3s Nov 28, 2023

IngelaAndin left a comment

u3s Nov 28, 2023

u3s Nov 28, 2023

Maria-12648430 commented Nov 29, 2023

IngelaAndin commented Nov 29, 2023

github-actions bot commented Nov 29, 2023 •

edited

Loading

Maria-12648430 commented Nov 29, 2023

IngelaAndin Nov 29, 2023 •

edited

Loading

Maria-12648430 Nov 29, 2023

Maria-12648430 Nov 29, 2023

IngelaAndin Nov 29, 2023 •

edited

Loading

Maria-12648430 Nov 29, 2023

Maria-12648430 Nov 29, 2023

IngelaAndin Nov 29, 2023

Maria-12648430 Nov 30, 2023

IngelaAndin Nov 30, 2023

Maria-12648430 Nov 30, 2023

u3s commented Nov 29, 2023 •

edited

Loading

Maria-12648430 commented Nov 29, 2023

u3s left a comment

IngelaAndin commented Nov 30, 2023

Maria-12648430 commented Nov 30, 2023

Maria-12648430 commented Nov 30, 2023

IngelaAndin commented Dec 4, 2023

Add documentation for {fail,Reason} to ct test case return values #7869

Add documentation for {fail,Reason} to ct test case return values #7869

Conversation

Maria-12648430 commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IngelaAndin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maria-12648430 commented Nov 29, 2023

IngelaAndin commented Nov 29, 2023

github-actions bot commented Nov 29, 2023 • edited Loading

CT Test Results

Artifacts

Maria-12648430 commented Nov 29, 2023

IngelaAndin Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IngelaAndin Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

u3s commented Nov 29, 2023 • edited Loading

Maria-12648430 commented Nov 29, 2023

u3s left a comment

Choose a reason for hiding this comment

IngelaAndin commented Nov 30, 2023

Maria-12648430 commented Nov 30, 2023

Maria-12648430 commented Nov 30, 2023

IngelaAndin commented Dec 4, 2023

Add documentation for `{fail,Reason}` to ct test case return values #7869

Add documentation for `{fail,Reason}` to ct test case return values #7869

github-actions bot commented Nov 29, 2023 •

edited

Loading

IngelaAndin Nov 29, 2023 •

edited

Loading

IngelaAndin Nov 29, 2023 •

edited

Loading

u3s commented Nov 29, 2023 •

edited

Loading