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

Autodetect CT SUITES #78

Merged
merged 1 commit into from Jul 25, 2014
Merged

Conversation

sedrik
Copy link
Contributor

@sedrik sedrik commented Jul 10, 2014

This change should enable autodetection of CT Suites. I have tried it on one personal project and it seems to work.

@essen
Copy link
Member

essen commented Jul 10, 2014

Almost. The suite name in CT_SUITES still must not include _SUITE.

Otherwise yes, definitely yes. That's one point that needed to be done for 1.0. On the other hand once the above change is done it will also need to be done in plugins/ct.mk for the new version (can be tested by generating it using 'make').

@sedrik
Copy link
Contributor Author

sedrik commented Jul 10, 2014

Any particular reason for needing to remove the _SUITE part? I tried it and it seems to work anyway. I can still look into doing it if you want.

@sedrik
Copy link
Contributor Author

sedrik commented Jul 11, 2014

I modified my change to do CT_SUITES ?= $(shell find test -type f -name \*_SUITE.erl -exec sh -c 'basename "$0" | cut -d'_' -f1' {} \;) instead but for some reason make only sees empty lines from that command but it works well in the shell. Do you have any insight into why this is or any other approach for how to do this.

@sedrik
Copy link
Contributor Author

sedrik commented Jul 14, 2014

There now it extracts the module name from the tests in both the old and new setup. I also updated the documentation to mention the autodetection. Is there anything else you need from me for this PR? =)

then you only need to put `ponies` in the list.
then you only need to put `ponies` in the list. This is usefull if you
need erlang.mk to skip some tests as the default is to autodetect your
common_test suites.
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the first sentence, say what the default is in the second sentence, and then explain how to override the default in the third sentence.

@essen
Copy link
Member

essen commented Jul 20, 2014

Still need the docs change and we're good to merge.

@sedrik
Copy link
Contributor Author

sedrik commented Jul 21, 2014

See if you like that better =)

@essen
Copy link
Member

essen commented Jul 21, 2014

There's a typo! And then squash the commits and we're good. Thanks!

@@ -232,7 +232,7 @@ CT_RUN = ct_run \
-dir test \
-logdir logs

CT_SUITES ?=
CT_SUITES ?= $(subst _SUITE.erl,,$(shell find test -type f -name \*_SUITE.erl -exec basename {}))
Copy link
Member

Choose a reason for hiding this comment

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

My bad, this here needs the ; at the end or -exec can't find its argument. Please put it back.

@essen
Copy link
Member

essen commented Jul 22, 2014

Also would be better to sort the suites by alphabetical order, they're not sorted at all right now.

@sedrik
Copy link
Contributor Author

sedrik commented Jul 22, 2014

Now it sorts as well.

@essen
Copy link
Member

essen commented Jul 23, 2014

Right but you need to put ; back in again, you removed it somehow.

@sedrik
Copy link
Contributor Author

sedrik commented Jul 24, 2014

ah, missed your comment about needing it. will fix tonight.

@sedrik
Copy link
Contributor Author

sedrik commented Jul 24, 2014

There semicolons are back.

@essen essen merged commit 2fa2981 into ninenines:master Jul 25, 2014
@essen
Copy link
Member

essen commented Jul 25, 2014

And merged, thanks!

@essen
Copy link
Member

essen commented Jul 27, 2014

You introduced a bug if test doesn't exist:

find: `test': No such file or directory

Please send a fix!

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.

None yet

2 participants