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

Fix use of ct on projects without CT suites and configuration #2299

Merged
merged 3 commits into from
Jun 10, 2020

Conversation

tolbrino
Copy link
Contributor

@tolbrino tolbrino commented Jun 5, 2020

Before, running rebar3 ct on such an empty project would crash with an
internal badmatch since no directory to run CT on could be found.

Now this case will terminate with a sensible error message and won't try
to run any further.

Before, running `rebar3 ct` on such an empty project would crash with an
internal badmatch since no directory to run CT on could be found.

Now this case will terminate with a sensible error message and won't try
to run any further.
@tolbrino
Copy link
Contributor Author

tolbrino commented Jun 5, 2020

I haven't fixed the tests yet, because I would like input whether the approach is correct. An alternative would be to let rebar3 ct succeed but not try to run any tests at all.

@ferd
Copy link
Collaborator

ferd commented Jun 5, 2020

The cleanest way to go would probably to give a success result with 0 tests run -- consider the failure to only trigger when tests fail (rather than only call it a success when they all pass). That would match what EUnit currently does, and compose the best with sequences like rebar3 do eunit, ct, cover put in templates for example.

@tolbrino
Copy link
Contributor Author

tolbrino commented Jun 5, 2020

The cleanest way to go would probably to give a success result with 0 tests run -- consider the failure to only trigger when tests fail (rather than only call it a success when they all pass). That would match what EUnit currently does, and compose the best with sequences like rebar3 do eunit, ct, cover put in templates for example.

Makes sense and while writing the PR I had the same feeling. Will adapt it.

@@ -339,6 +341,16 @@ multi_app_ct_macro(Config) ->
true = lists:member({d, 'COMMON_TEST'}, ErlOpts)
end, Apps).

no_ct_suite(Config0) ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ferd This test fails currently with {error, interactive_mode} while running the same setup from the command-line succeeds. I haven't found the culprit in the ct provider yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this related to https://erlang.org/doc/apps/common_test/run_test_chapter.html#running-the-interactive-shell-mode ? I never managed to make that work properly or get anything to work with it since the instructions were kind of obscure.

It's possible that was failing way before this test case I figure.

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 is the one, although I don't see where the mode gets activated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I don't know. Curious if passing in a specific lack of options triggers the mode implicitly but I don't know why it would not trigger in the command-line vs others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out running CT in an existing CT context is internally considered running in interactive mode. This explains why it works standalone. I adapted the test to only ensure that the options are properly set up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hah, tricky one.

@ferd ferd merged commit a913070 into erlang:master Jun 10, 2020
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