Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kuba/ct/ct hooks order option/otp 18682 #7496

Merged
merged 6 commits into from
Jul 24, 2023

Conversation

u3s
Copy link
Contributor

@u3s u3s commented Jul 13, 2023

No description provided.

@u3s u3s added the team:PS Assigned to OTP team PS label Jul 13, 2023
@u3s u3s self-assigned this Jul 13, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 13, 2023

CT Test Results

    1 files    11 suites   5m 11s ⏱️
  93 tests   91 ✔️ 2 💤 0
109 runs  107 ✔️ 2 💤 0

Results for commit abde78f.

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

@u3s u3s added the testing currently being tested, tag is used by OTP internal CI label Jul 13, 2023
Copy link
Contributor

@garazdawi garazdawi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good and from what I can tell they also work :)

While digging in the code I noticed that the ct_hooks option can be given to a lot of more places than we originally intended. The CTH User's Guide I wrote a long time ago restricts it to three function, but the original implementation seems to have allowed it (inadvertently) in a lot of other places which through the years have become documented, though I can't find any tests for it.

Not much we can do about that now, but I'm not sure we need to make the same mistake for ct_hook_order. IMO it should be enough to allow that option in the suite callback and in test specs/command line.

@u3s
Copy link
Contributor Author

u3s commented Jul 17, 2023

Not much we can do about that now, but I'm not sure we need to make the same mistake for ct_hook_order. IMO it should be enough to allow that option in the suite callback and in test specs/command line.

I will gladly reduce supported callbacks to suite. I tried to follow ct_hooks assuming it is all well thought and needed. I didn't dare to think it is all an accident ;-)

Also on 2nd thought, I think ct_hooks_order is more 'global' thing in framework than ct_hooks.

@u3s u3s force-pushed the kuba/ct/ct_hooks_order_option/OTP-18682 branch 2 times, most recently from ccead74 to 434abbf Compare July 18, 2023 12:42
- support ct_hooks_order only in cmd line, spec and suite/0
@u3s u3s force-pushed the kuba/ct/ct_hooks_order_option/OTP-18682 branch from 434abbf to abde78f Compare July 19, 2023 08:17
@u3s u3s removed the testing currently being tested, tag is used by OTP internal CI label Jul 24, 2023
@u3s u3s merged commit f0e8d00 into erlang:master Jul 24, 2023
12 of 15 checks passed
@u3s u3s linked an issue Jul 24, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix team:PS Assigned to OTP team PS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CT executes post_* hooks in the wrong order
3 participants