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

Common Test SUITE template #2070

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

max-au
Copy link
Contributor

@max-au max-au commented May 9, 2019

Contains boilerplate code and a few hints how Common Tests are
to be used

{name, "suite", "Name of the suite, prepended to the standard _SUITE suffix"}
]}.
{dir, "test"}.
{template, "ct_suite.erl", "test/{{name}}_SUITE.erl"}.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is one of the problems we have overall with the templater. It only works from the current working directory, and we get a bit of a funny thing for umbrella releases where the tests might go in appname/test/{{name}}_SUITE.erl.

Can you quickly check if we can allow {{dir}}/{{name}}_SUITE.erl as a path but with dir configurable with a default to test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was also going to add a feature for getting the name of the parent directory to use. I need that to complete the init-release template so it has a good default name. Could use the same for this.

init_per_suite/1, end_per_suite/1,
init_per_group/2, end_per_group/2,
init_per_testcase/2, end_per_testcase/2
]).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any opinions on using -compile(export_all). just for the CT suite as Tristan initially suggested in the issue? I know that out of laziness I often revert to that one just for CT suites as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd vote against export_all. Not only it requires to suppress compiler warning, but also creates "accidental interface" that works in test mode only.

So far I've been happy with just evaluating the code (e.g. using power_shell, https://github.com/WhatsApp/power_shell)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a test module, it only runs in test mode :). And doesn't really have an interface because it isn't called by anything but the ct runner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That one is a test module, but text message suggests to suppress "export_all warning" for the entire project compiled with test profile.
I agree, export_all for test SUITE makes sense, I just wish it's possible to enable warning suppression it only for test/ folder of the project.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't -compile([export_all, nowarn_export_all]). prevent warnings only in this module? It seems to work well on a small shell test here, but I haven't checked across all versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@max-au true, but a regular compile would still have the warning and building like a release would then still warn or error so it couldn't go to prod.

But point taken and looks like Fred has a solution.

@tsloughter
Copy link
Collaborator

I finished one yesterday too and forgot to open it. it adds additional functionality to rebar3 new one minute.

@tsloughter
Copy link
Collaborator

Personally not a fan of so many comments in a template. But I'm fine with being out voted on it and then we can just merge our PRs #2072 :)

@garazdawi
Copy link
Contributor

Personally not a fan of so many comments in a template.

Comments are great if it is the first time you see a template, but when it is the 100th they are just in the way. So IMO it depends on whether you want to help first time users or experienced developers more.

@tsloughter
Copy link
Collaborator

tsloughter commented May 10, 2019

@garazdawi my thoughts exactly. I figure the first time the user can use http://erlang.org/doc/apps/common_test/example_chapter.html

We could put a link to that in the template at the top. Or in the new usage output that is added in my PR.

@max-au
Copy link
Contributor Author

max-au commented May 10, 2019

Agree, comments should be removed, let me update it. Not sure about export_all though, and compiler warning suppression.

@tsloughter
Copy link
Collaborator

I'm more open to the comments than I am to a full list of functions :).

Contains boilerplate code and a few hints how Common Tests are
to be used
@max-au max-au force-pushed the common_test_suite_template branch from f75dbc1 to b81aa63 Compare May 10, 2019 14:36
@ferd ferd mentioned this pull request Jun 8, 2019
@tsloughter
Copy link
Collaborator

@ferd @max-au I had a thought.

What if we added the CT suite with all the comments as a part of the app, lib and release/umbrella templates? So when the user generates a full project they get a detailed CT suite. But when generating a suite on its own a simpler template is used.

It looks like this one was updated to be simpler already so would be the one used in the latter?

@ferd
Copy link
Collaborator

ferd commented Aug 27, 2020

Yeah that sounds good to me. Showing to people where to put their tests in the initial template sounds good, and we can afford more docs into that one.

@max-au
Copy link
Contributor Author

max-au commented Aug 28, 2020

I like this suggestion too.
But I'm opposed to "export_all" (we had quite a history with it), and even more against suppressing the warning about it.

@tsloughter
Copy link
Collaborator

@max-au can you revert to the original template you had and add it to the app.template, lib.template, escript.template, plugin.template, release.template and umbrella.template template files.

I'd prefer it included:

-compile(nowarn_export_all).
-compile(export_all).

But I suppose it doesn't have to.

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.

4 participants