dlangbot.github: Show Digger test command in automatic message

[CyberShadow]

The previous message only told users how to build their pull request, but not how to run the test suite.

There's a few relevant commands here:

• checkout just gets the source code + does any merges necessary.
• build gets the source code, builds it, then puts it in a directory ready to be added to PATH and used.
• rebuild just builds whatever's currently checked out.
• test runs the test suite of whatever's currently checked out.

build + test is probably the most useful combination, as it allows both running the test suite, and giving users a build that they can play around with (i.e. test it against their personal codebase).

[dlang-bot]

Thanks for your pull request, @CyberShadow!

[wilzbach]

I don't have a strong opinion on this, but I'm not so sure users will actually find this useful (and space comes at a premium here), because in my experience no one except for the author runs the testsuite for a PR locally and after all we have a few CIs configured to do so which will show warnings if something went wrong...

[CyberShadow]

Well, what bugs me is that the message is currently kind of misleading: it's saying "you can use Digger to test this PR", but the command actually doesn't do any testing.

[PetarKirov]

I'm strongly in favor, thanks @CyberShadow!

[MartinNowak]

The comment could be improved somewhat to be more relevant and terse.
How about only adding the bugzilla section only if the word fix appears in one of the commits or the PR title?
How about leaving the basic digger command, but linking to a landing page in Digger's wiki for more possibilities?
I don't think the word test here actually means unit-testing, to me it seems more targeted at people trying out a feature/fix on their own code/examples.

[wilzbach]

How about only adding the bugzilla section only if the word fix appears in one of the commits or the PR title?

We already do that - it's shown only when at least one issue has been referenced.

[MartinNowak]

I meant this part, might be useful but takes quite some space. Low prio though.

[wilzbach]

I meant this part, might be useful but takes quite some space. Low prio though.

I see, but that's intended because many people forget to reference issues (especially newcomers) and about 80% of all PRs should either reference an issue or contain a changelog entry.
Though of course, it's a bit annoying for all of Walter's refactoring.
Maybe we can only enable it for non D team members (in a similar fashion to how we already shorten the message for team members)?

[CyberShadow]

Ping, where do we go from here?

and space comes at a premium here

One way we can conserve vertical space is the <details> tag, which creates a collapsed / expandable section.

in my experience no one except for the author runs the testsuite for a PR locally

It's useful if a test fails, and it's not clear why from just the CI log. You'd need to add debug code to the implementation or debug/trace the failing test.

As an alternative to this change, we could:

• change the wording to remove the word "test" and fix the false implication that the Digger build command runs any tests
• change something in Digger? I don't see how to put everything in one command though, as there is the critical distinction between an initial setup (checking out the PR code) and a repeated action (edit -> build + run tests -> repeat).

[wilzbach]

It's useful if a test fails, and it's not clear why from just the CI log. You'd need to add debug code to the implementation or debug/trace the failing test.

Yes, but in most cases it's a lot easier to run the failing DMD test yourself via the e.g. test/run.d or the Phobos testfile via the .test target as this only takes seconds (at most).

• change the wording to remove the word "test" and fix the false implication that the Digger build command runs any tests

Yeah we should definitely replace the word "build" (or reword the entire sentence + header).
I don't expect anyone to download a PR and run the whole testsuite (that's what we have the CIs for). When I added the instructions my intention was to provide easy instructions for people to experiment with a new feature/bug fix locally and provide feedback on it.

where do we go from here?

I would suggest to reword the sentence + heading, s.t. it's clear that this provides a easy way to play/experiment with the respective PR locally and it's not about testing it. What do you think?

and space comes at a premium here

A bit unrelated, but the hopefully upcoming dub run (dlang/dub#1428) will save one line too (though we would have to wait a few versions until it can be displayed here).

One way we can conserve vertical space is the <details> tag, which creates a collapsed / expandable section.

We already link to Digger, so they could also click on that link for more info ;-)

[CyberShadow]

Yes, but in most cases it's a lot easier to run the failing DMD test yourself via the e.g. test/run.d or the Phobos testfile via the .test target as this only takes seconds (at most).

Does that take care of setting up all the dependencies and the environment, esp. on Windows? I didn't see anything for setting up MSVC or DMC / DM tools in there.

[wilzbach]

Does that take care of setting up all the dependencies and the environment, esp. on Windows?

test/run.d is a cross-platform replacement for Make (so no DM is needed here anymore). The only problem on Windows are the ~50 bash tests which would fail if run out from a normal Windows shell and not e.g. the git shell.
There's also src/build.d for building on Windows without DM or Visual Studio, but as no respective make-free build scripts exist for druntime + phobos one doesn't get too far without DM make.

Anyhow, my point was more that if you submit a PR to DMD, you should already have figured out how to build DMD locally or when making a PR to Phobos then you should have managed to build Phobos locally. And editing the source files in place has the advantage that they can directly git commit their new changes.
I think the more common problem is that a specific test is failing on Windows or OSX and the author doesn't have access to such a platform to be able to run the tests.

[CyberShadow]

And editing the source files in place has the advantage that they can directly git commit their new changes.

You can still do that, though? Digger checks out the code in a git repository, so you can commit and push from there, you just need to add your remote.