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

Diff #1

Closed
wants to merge 17 commits into from
Closed

Diff #1

wants to merge 17 commits into from

Conversation

arcusfelis
Copy link
Collaborator

Added tests for ct hook.
Do not merge (just for diffing)

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

The code looks good. Tests are a bit difficult to grasp, but I think adding a few comments would make them better (see comments). I would clean up the commit history a bit to make it cleaner and more descriptive.

{ok, GrSum} = file:consult(GrDir),
#{sum => Sum, gr_sum => GrSum}.

% ct_app/_build/test/logs/last/all_groups.summary
Copy link
Member

Choose a reason for hiding this comment

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

Are these lines some leftovers? I don't see how they could be helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, they are cats of these two files, to show the content, but I guess it is now in test cases, so could be removed.

@@ -0,0 +1,63 @@
-module(test2_SUITE).
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to have at least some comments about the suites. Names like test_SUITE, test2_SUITE, test3_SUITE are not helpful...

failing_tc_2,
failing_tc_3].

% init_per_suite(_Config) -> ?ERROR({error, init_per_suite});
Copy link
Member

Choose a reason for hiding this comment

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

There are some commented out lines like this one... maybe remove them?

%% when they are no longer referenced
[ {counter, atomics:new(1, [{signed, false}])} | Config ].

% end_per_suite(_Config) -> ?ERROR({error, end_per_suite});
Copy link
Member

Choose a reason for hiding this comment

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

Here as well

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

Looks ok 👍

@arcusfelis
Copy link
Collaborator Author

It is already in master, there was some issues creating a first commit :)
Tip for future: create new repos with empty first commit :)

@arcusfelis arcusfelis closed this Aug 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants