Fix example code in ct_hooks_chapter.xml

[fabjan]

Problem

When I copied the example hook from the Common Test User's Guide to use as a base for making a new hook
my editor complained about the code.

The example code has a syntax error (missing a period), and uses the deprecated erlang:now/0 function.

Fix

I followed the suggestions in "How to Work with the New API" to replace the erlang:now/0 calls.

[CLAassistant]

All committers have signed the CLA.

[github-actions]

CT Test Results

    3 files    69 suites   1h 30m 51s ⏱️
558 tests 545 ✔️ 13 💤 0 ❌
599 runs  583 ✔️ 16 💤 0 ❌

Results for commit 0631c7f.

♻️ 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

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[u3s]

Thanks for taking a deeper look into that area!

I like the reduced [minimal example] (https://gist.github.com/fabjan/754840a6daf30058c00880b465448974). I think current example is more of a code skeleton than a minimal example.

I would suggest having it(minimal_example) in ct_hooks_chapter with following adjustments:

1. including underscored variable name - e.g. _Suite says more than _
2. leaving one of on_tc_* callbacks (even if empty) to indicate the presence of that callback category
3. I agree using a file in common hook is not a robust solution and has it disadvantages, but maybe it could be left alone with a small explanation of the limitations in the code comment. my thinking is that a file is one of easiest things to have with some observable side effects at the same time - at that is the main purpose of minimal example.
4. I'm not sure about exact problems which you have. I tried following and it worked:
   • take your minimal example
   • compile it with erlc example_cth.erl
   • create example_SUITE.erl from "Small Common Test suite" skeleton under Emacs
   • run suite with a hook with
     ct_run -suite example_SUITE -pa . -ct_hooks example_cth
   • Above worked even when /tmp/file.log was opened in Emacs.
5. add example command for trying hook module out (as a comment in minimal_example code)
   ct_run -suite example_SUITE -pa . -ct_hooks example_cth

Additionally (does not have to be included in this PR), I thought about following:

1. while at it, I think it would be worth adding a note in this section explaining that a hook beam file needs to be added to code path to install it, therefore using -pa . might be needed

I brought points 1 and 5 because regularly when I'm explaining CT hooks to students I got a question/suggestion that feature does not work when actually the forget to adjust the code path or compile the hook module before hand.

3. The example in current shape (with callbacks doing nothing) could be considered more of a template/skeleton and maybe could fit nicely in https://www.erlang.org/doc/apps/common_test/example_chapter.html ?
4. If you would like to be nice to Emacs users and know how to test it (a template code skeleton) - maybe you could also add a skeleton code erlang-skels.el

[fabjan]

Cheers for the feedback! I'll take a stab at pulling the minimal example in here (and look at the other files you mentioned, I always want to be nice to Emacs users).

I'm not sure about exact problems which you have. I tried following [...]

Yes it works when installing the hook via e.g. the CT command line, but when the hook is installed by a spec/1 callback in the SUITE, the version of the hook that opens the file on init and writes at the end gets io errors for me. If the open is moved to the end just before the write, the errors go away but if the mode append is included no data is lost (but there is one summary per hook install so it seems the init happens multiple times, even if the same hook ID was returned at the start.

[u3s]

1. for issues with running hook from spec - can you check the pids executing each hook callback and if the one executing init is alive in terminate ?- AFAIK, they might differ. if process opening file is not alive anymore when terminate is executed you might face issues (hard to say why this is not visible when hook is installed via command line, I see process differ as well for each callback).

• if this improves robustness and is aligned with minimal concept of the example - maybe file should be opened and closed in terminate callback (example hook is collecting execution data in the memory anyway)
• I've noticed in your repo that you're installing 2 hooks which is of course fine - but please check if the hooks code doesn't interact in unwanted manner
• see shell interaction below (I think it illustrates your error)

2. BTW, please check indentation (so 4 spaces are used)

1> {ok, Device} = file:open("/tmp/file.log", [write]).
{ok,<0.86.0>}
2> self()
2> .
<0.84.0>
3> io:format(Device, "Hello", []).
ok
4> 1/0.
** exception error: an error occurred when evaluating an arithmetic expression
     in operator  '/'/2
        called as 1 / 0
5> self().
<0.90.0>
6> Device.
<0.86.0>
7> io:format(Device, "Hello", []).
** exception error: terminated
     in function  io:format/3
        called as io:format(<0.86.0>,"Hello",[])
        *** argument 1: the device has terminated

[fabjan]

if process opening file is not alive anymore when terminate is executed you might face issues (hard to say why this is not visible when hook is installed via command line, I see process differ as well for each callback).

It might be because I installed the hook multiple times, which I did on purpose to see how it works aside from reading the docs. The docs say that if you install multiple times as long as the ID is the same the second hook should not be started at all(?): "The Id identifies a CTH instance uniquely. If two CTHs return the same Id, the second CTH is ignored and subsequent calls to the CTH are only made to the first instance. For details, see section [Installing a CTH] in the User's Guide."

• if this improves robustness and is aligned with minimal concept of the example - maybe file should be opened and closed in terminate callback (example hook is collecting execution data in the memory anyway)

I would vote for this, given that this is supposed to be a minimal example I think as long as it works and makes sense it's good (however much my vote counts in this repo, just drive-by PR:ing some docs 🙂). As you say the state is in memory, so it's hard to tell if the file open is in init on purpose or by happenstance. We can also add the append flag to the example. So in case someone starting off from the example hook manages to install them multiple times it is more obvious (double output instead of some "randomly" lost output).

Also, if we can decide to just do this (mode the file open form init to the end before write), we don't have to debug this minimal example hook code more (the opened file handle messing up when I used multiple hooks). If it is not in init on purpose, and the current example doesn't work as advertised it seems to me to be harmless to "fix" it even if we don't know the deepest root cause. Because the example code in the current documentation does not work as the docs advertise, even after syntax errors are fixed: installing the hook via the TestSuiteModule:suite/0 yields the io errors I mentioned, and even if the file open is moved to the end the different suites write over each other, so it seems even if the hook id is the same, multiple hooks are running and keep their own state. By adding append to the flags when opening, they at least don't overwrite each other.

But perhaps I misunderstood something in the docs.

(This is the reason I pulled the code out into that separate repository, to poke at it for real instead of inside an HTML document. Thanks for looking at that code in addition to just inside this PR, I think that makes things much easier.)

I will make sure to not change the style (indentation in this case) when updating this PR 👍

[fabjan]

Side note about the indentation. As the file is now, it seems to be using a four space indent level BUT all lines are prefixed with one extra space.

I left the extra space at the beginning to try and make sure the diff for my changes is a as legible as possible, showing only the meaningful changes. Would you like me to in this PR also get rid of that first extra column of space characters? (Maybe the diff preservation is not as interesting now that we are discussing bigger changes anyway)

[fabjan]

I added the minimal example hook now, with the adjustments discussed above. I think I have addressed all the suggestions above, either in the code or in my comments here (except for the emacs skeleton, will look at that next).

For the sake of the review, may I suggest that any further issues with the code changes here are discussed under the "Files changed" tab? I think that makes it easier to manage the discussions as separate threads when the issues are separate.

The indentation question I think is not quite resolved, see my side note comment above.

[fabjan]

I put the suggested Emacs skeleton in this PR as well, we can remove it if you prefer.

Tested by putting this in my ~/.emacs.d/init.el:

;; Set up Erlang mode
(setq load-path (cons "~/code/otp/lib/tools/emacs" load-path))
(require 'erlang-start)

The skeleton showed up in the "Erlang" menu, and produces this:

I copied that module and tested in my example_cth repository (linked in the thread above) and it logged this:

=NOTICE REPORT==== 6-Dec-2022::21:15:26.870679 ===
test_hook is done: {state,4,2,0,1,0}

[u3s]

Sorry for delay. This PR has lower priority and must wait longer.

1. Please fix minor comments I've added in code.
2. can you set target of PR to master branch, please?
3. I've played around with adding hook modules via ct_run parameter and from suite/0 function in test suite. My observations are that, Module:init/2 behaves as expected (i.e. if the hook module returns the same id - init will be called only once). There are more calls to Module:id/1, but I presume this is needed for checking hook ID by the framework and making the decision.

So from my perspective, there is nothing suspicious at this stage. However if you still observe some anomaly worth checking, please create separate issue with reproduction steps and we will try to clarify that.

[u3s]

Please avoid Latin acronyms in docs so it is more accessible. Not everyone was privileged to have Latin classes at school ;-)

[fabjan]

Ah, no problem! Will fix. How about I just write "Note: " instead?

[u3s]

Good enough!

[u3s]

I left the extra space at the beginning to try and make sure the diff for my changes is a as legible as possible, showing only the meaningful changes. Would you like me to in this PR also get rid of that first extra column of space characters? (Maybe the diff preservation is not as interesting now that we are discussing bigger changes anyway)

Please remove the extra space in the example code. We will see how it behaves ... it looks artificial, maybe there is some reason I'm missing ... let us check and hope it will be fine:-)

[fabjan]

👍

[fabjan]

Sorry for delay. This PR has lower priority and must wait longer.

No worries, that makes sense.

1 and 2: will fix and update PR.

3: yeah I agree it should be treated separately, I think that's something I experienced when I was trying the file handle issue out in different ways. If I can reproduce something meaningful I can open a separate issue.

[fabjan]

The suggestions should all be adressed now.

[fabjan]

The build failed after curling the GitHub API into jq: https://github.com/erlang/otp/actions/runs/3741792360/jobs/6353967342#step:8:6181

Since it's inside a shell pipeline we can't see what the cause was, but it's possible curl -sL "https://api.github.com/repos/$1/tags"was rate limited. If you authenticate that call its rate limit will increase from 60 per hour to 1000 per hour (as per current GitHub API policy).

[u3s]

I've pushed small commit removing some spaces and wrapping code line. I wanted to check if we can get rid of scrollbar under code example and improve readability.

If it it works and you are fine with this suggestion - please squash the commits on the branch and we will merge it.

[fabjan]

I'm fine with that formatting 👍, but not sure how to check if it renders better. I'll squash the commits though.

[u3s]

I'll squash the commits though.

Thanks

... but not sure how to check if it renders better.

easiest way I'm aware of is to navigate to HTML doc generated for PR. once processed link should appear in area visualized on screenshot below (it usually works) - "No HTML docs found" should be replaced with a link.

[u3s]

no horizontal scroll bar now :-). at least in my browsers.

https://erlang.github.io/prs/6437/lib/common_test-1.23.2/doc/html/ct_hooks_chapter.html#example-cth

[fabjan]

Nice, yeah for me too 👍