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

Add: recursive option on test command #1873

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

Add: recursive option on test command #1873

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 7, 2020

No description provided.

@dlang-bot
Copy link
Collaborator

Thanks for your pull request and interest in making D better, @Sobaya007! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

@thewilsonator
Copy link
Contributor

Thanks! could you please add a changelog entry for this.

@wilzbach
Copy link
Member

wilzbach commented Feb 7, 2020

... and test ;-)

@andre2007
Copy link
Contributor

Yes, the test is important. I am not sure wheter it works as expected because the dependencies are compiled not in unittest mode when running dub test on a dub package.

Also what is the expected behavior if the test of a dependency fails, will the unittests for the other dependencies still be executed?

@ghost
Copy link
Author

ghost commented Feb 7, 2020

How can I add test?
Write some unittest block or putting any shell script in test directory?

@wilzbach
Copy link
Member

wilzbach commented Feb 7, 2020

some unittest block or putting any shell script in test directory?

Yes in test most of the existing tests can be found. They are mostly shell scripts. You could check for the tests for dub add if you need help on how to do it exactly.

@andre2007
Copy link
Contributor

If I am not wrong, there is a *.sh file missing which call dub test -r for the test package you added.

@ghost
Copy link
Author

ghost commented Feb 8, 2020

According to run-unittest.sh, just adding package directory into test will be OK.
Actually I ran travis.sh on my PC and confirmed the test is executed.

@andre2007
Copy link
Contributor

I can't see how your tests triggers your new recursive functionality. Where do you pass the --recursive argument?

@ghost
Copy link
Author

ghost commented Feb 8, 2020

Oh, sorry. It's my mistake.

@@ -1009,6 +1010,9 @@ class TestCommand : PackageBuildCommand {
args.getopt("f|force", &m_force, [
"Forces a recompilation even if the target is up to date"
]);
args.getopt("r|recursive", &m_recursive, [
"Runs test on all packages including subpackages."
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this command-line help seems different from fact.
This option recursive tests for dependencies, not subpackages.
(I'd rather test the subpackages recursive.)

@andre2007
Copy link
Contributor

A proposal: the test libraries could output in unittest mode a text to stdout. In your test bash script you can grep for this text. Or even better call one time dub test with -r and one time without -r and compare the output.

A sample how to to grep and compare dub output in bash you can see here https://github.com/dlang/dub/pull/1811/files

@andre2007
Copy link
Contributor

Thanks. I thought of s.th. like this in the test script:
Pseudo code

dub test root_package -r | output contains root_pack and liba and libb and libc
dub test root_package | output contains only root_pack

@ghost
Copy link
Author

ghost commented Feb 22, 2020

Anything else?

@andre2007
Copy link
Contributor

Only a nitpick, I am not sure whether 4 additional git ignore files are needed. Also please merge the 11 commits into 1 commit.

@andre2007
Copy link
Contributor

Otherwise looks good for me. Thanks a lot for your effort.

@ghost
Copy link
Author

ghost commented Feb 22, 2020

OK?

@Panke Panke mentioned this pull request Mar 16, 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.

6 participants