Skip to content

Conversation

@ikavgo
Copy link
Contributor

@ikavgo ikavgo commented May 25, 2025

As a follow-up to my GChat thread about removing default logger handler to clean CT stdout, I was looking at
injecting logger config with undefined default handler to ct_run. It is possible but breaks cth_styledout - no nice green things whatsoever. Then I found rabbit_ct_hook which calls redirect_logger_to_ct_logs which in turn calls logger:remove_handler(default) apparently with zero effect! To cut story short - turned out rabbit_ct_hook must run before cth_styledout for remove_handler line to have any effect.

Doesn't work for parallel targets yet -

# @todo We must ensure that the CT_OPTS also apply to ct-master
But for manual local runs like make -C deps/rabbit ct-<> it really shines.

…er properly removed

As a follow-up to my GChat thread about removing default logger handler to clean CT stdout, I was looking at
injecting logger config with undefined default handler to ct_run. It is possible but breaks cth_styledout - no
nice green things whatsoever. Then I found rabbit_ct_hook which calls redirect_logger_to_ct_logs which in turn
calls logger:remove_handler(default) apparently with zero effect! To cut story short - turned out rabbit_ct_hook
must run before cth_styledout for remove_handler line to have any effect
@mergify mergify bot added the make label May 25, 2025
@ikavgo ikavgo marked this pull request as draft May 25, 2025 17:09
@ikavgo
Copy link
Contributor Author

ikavgo commented May 25, 2025

@michaelklishin It works for individual suites, but not for parallel - too much magic. I can add rabbit_ct_hook to other deps if this approach is fine.

@michaelklishin
Copy link
Collaborator

@ikavgo I certainly find this output an improvement. We can start with the core (deps/rabbit, deps/rabbit_common) and rabbitmq_management, rabbitmq_mqtt, and then switch more subprojects over time.

@michaelklishin
Copy link
Collaborator

Unless @dumbbell @lhoguin have objections, I think we can extend this to a few subprojects with large test suites (see above) and merge this change for main, then see how it goes.

@lhoguin
Copy link
Contributor

lhoguin commented May 26, 2025

Fine with me.

@dumbbell
Copy link
Collaborator

Looks good to me as well.

@michaelklishin michaelklishin marked this pull request as ready for review May 26, 2025 07:40
@ikavgo
Copy link
Contributor Author

ikavgo commented May 26, 2025

cool, I will add to rabbit_common, management and mqtt.

@lhoguin any hints on parallel suites? It looks like it has this "inplace" ct_master right inside the Makefile =). I feel like I plug CT_OPTS somewhere here, but not sure where still...

@lhoguin
Copy link
Contributor

lhoguin commented May 26, 2025

Options need to be converted to their Erlang term equivalent and given to CT from the Erlang script in the Makefile. Not sure about the details.

@ikavgo
Copy link
Contributor Author

ikavgo commented May 26, 2025

I added crude support for parallel test (ct_master is forked anyway), and replaces some ct:pals with ct:log.

Before: https://github.com/rabbitmq/rabbitmq-server/actions/runs/15240078576/job/42860845449#step:9:13156
After: https://github.com/rabbitmq/rabbitmq-server/actions/runs/15254128554/job/42897594880

ikavgo added 3 commits May 26, 2025 16:57
Helps cleaning-up/coloring stdout for parallel targets
TODO: there are obvious races for different nodes outputs
In the next iteration I hope to implement cursor tracking for each node
@ikavgo ikavgo force-pushed the ik-rabbit_ct_hook branch from ce213e1 to 6b528e2 Compare May 26, 2025 14:57
@ikavgo
Copy link
Contributor Author

ikavgo commented May 26, 2025

I gave up on rabbit_ct_hook for rabbit_common. ct_helpers depend on rabbit_common and it is all sorts of ugliness

@michaelklishin
Copy link
Collaborator

@ikavgo no worries, we can leave rabbit_common as is for now. Most test suites are not in rabbit_common anyway :)

@ikavgo
Copy link
Contributor Author

ikavgo commented May 26, 2025

It is green, finally. If looks good, I think I'll add to the rest of the deps and if you are flying I can merge. Thoughts?

@michaelklishin michaelklishin merged commit 4c34155 into main May 26, 2025
539 of 540 checks passed
@michaelklishin michaelklishin deleted the ik-rabbit_ct_hook branch May 26, 2025 17:03
@michaelklishin
Copy link
Collaborator

michaelklishin commented May 26, 2025

Thank you, @ikavgo.

@michaelklishin michaelklishin added this to the 4.2.0 milestone May 26, 2025
michaelklishin added a commit that referenced this pull request Jun 4, 2025
LoisSotoLopez pushed a commit to cloudamqp/rabbitmq-server that referenced this pull request Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants