Simplify interface to rdmd_test and make it more robust by marler8997 · Pull Request #331 · dlang/tools

marler8997 · 2018-03-16T18:53:39Z

Support compilers with relative filenames
Remove the --rdmd-default-compiler argument since this can be pulled from rdmd help text.
This removes the need to specify this as an argument and is more DRY since this information is already known to the rdmd binary under test.
Since --rdmd is required, made it a "non-option" argument instead of an "option that is [REQUIRED]"
Change --test-compilers to --compilers and made it optional by defaulting to 'dmd'
Move non-compiler specific tests to a separate function so they only get tested once instead of once per compiler.

dlang-bot · 2018-03-16T18:53:39Z

Thanks for your pull request and interest in making D better, @marler8997! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
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.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

1. Since --rdmd is required, make it a non-option argument 2. Change --test-compilers to --compilers and make it optional by having it default to 'dmd' 3. Remove the --rdmd-default-compiler argument since this can be pulled from rdmd help text. This removes what was a required argument before making the command line interface simpler and the test more DRY. 4. Support compilers with relative filenames 5. Move non-compiler specific tests to a separate function so they only get tested once instead of once per compiler.

CyberShadow · 2018-03-17T20:27:25Z

CC @WebDrake

WebDrake

My initial inclination was to welcome simplifying rdmd_test to avoid the need for the --rdmd-default-compiler flag, but on balance I'm not sure that's a good idea.

The thing to understand here is that by explicitly specifying the default compiler, we create an extra test case beyond the fact that rdmd can fall back to its test compiler: we ensure that it falls back to the test compiler that we think it should have.

For example: suppose someone tweaked rdmd.d such that now defaultCompiler was always gdmd (a nasty bug). The fallback test in master would fail because the makefiles pass in the build compiler via the --rdmd-default-compiler flag. With the alternative setup in this PR, the test suite would parse gdmd from the help output, the assert would accept this as a valid fallback compiler name, the test suite would create a dummy gdmd, and the tested rdmd would indeed fall back to using it ... and so the test would pass. That's not what we want.

Changing this also means replacing a test that fails in simple ways (there's a mismatch between what the user specifies as the default compiler, and what rdmd thinks it is) with something that could fail in a number of different complex ways (e.g. bugs as described above, or changes to rdmd's help output, or changes to rdmd such that there are more options for what the default compiler could be).

So, I really don't think this is a good change. It's NOT making rdmd_test more robust: quite the opposite, in fact.

Apart from that, as submitted this PR conflates a bunch of extra concerns into the same patch:

bikesheddy stuff, in particular renaming the --test-compilers flag and the corresponding underlying variable
rearranging the test suite: breaking out tests that do not use an explicit --compiler= flag into runCompilerAgnosticTests (not a bad thought in its own right, but not relevant to the rest of the PR, and CompilerAgnostic seems a bit of a misleading name when it comes to the fallback test)
replacing the --rdmd flag with an expectation that the rdmd instance just be the first non-flag argument

Apart from the fact that these really should be broken out into separate patches (or ideally PRs), I would like to understand what the intended use-case is here.

I assume that the intent is to allow the user to run rdmd_test directly while typing less. But I'd like to understand why it seems necessary to run it directly inn the first place. After all, the whole point of many of the earlier changes was to ensure that developers shouldn't have to do that.

WebDrake · 2018-03-18T17:39:39Z

rdmd_test.d

 {
-    string rdmd; // path to rdmd executable
-    string defaultCompiler; // name of default compiler expected by rdmd
+    string compilers = "dmd"; // e.g. "ldmd2,gdmd" (comma-separated list of compiler names)


I would suggest not hardcoding a default here. In the absence of anything being specified, it should default to rdmd's default compiler.

WebDrake · 2018-03-18T17:41:47Z

rdmd_test.d

    auto helpInfo = getopt(args,
-        "rdmd", "[REQUIRED] path to rdmd executable to test", &rdmd,
-        "rdmd-default-compiler", "[REQUIRED] default D compiler used by rdmd executable", &defaultCompiler,
+        "compilers", "comma-separated list of D compilers to test rdmd with (defaults to 'dmd')", &compilers,


This is a superfluous change that honestly looks more like bikeshedding over what the test-compilers flag (and internal variable) are called. I'd suggest dropping it since it distracts from the more important behavioural changes in this patch.

WebDrake · 2018-03-18T17:52:35Z

rdmd_test.d

+    defaultCompiler = defaultCompiler[0 .. defaultCompiler.countUntil!isWhite];
+    if (verbose)
+       writefln("defaultCompiler is '%s'", defaultCompiler);
+    assert(defaultCompiler == "dmd" || defaultCompiler == "ldmd2" || defaultCompiler == "gdmd");


This should be enforce, not assert, since it's validating external input, and because we'd always want the check to be performed.

However, note that this is introducing a maintenance burden, because it now means that assumptions about the possible choices of default compiler name are hardcoded into rdmd_test.

WebDrake · 2018-03-18T18:10:14Z

rdmd_test.d

    {
        reportHelp();
-        return;
+        return 1;


It would be simpler to pass an error message to reportHelp like "rdmd binary not specified!". That would avoid having to care about return values (and changing the main function definition).

WebDrake · 2018-03-18T18:12:54Z

rdmd_test.d

+    if (args.length > 2)
+    {
+        writefln("Error: too many non-option arguments, expected 1 but got %s", args.length - 1);
+        return 1;


It would again be easier to report this error message via the reportHelp function (arguably it's nice to print the usage output alongside this error message and again, this avoids having to care about return values).

WebDrake · 2018-03-18T18:13:28Z

rdmd_test.d

    }

-    if (helpInfo.helpWanted)
+    if (args.length == 1 || helpInfo.helpWanted)


The various arguments-validation checks introduced here and below look harder to understand and maintain than just preserving the --rdmd flag. I'm not sure that the simplicity of being able to type

rdmd_test [PATH_TO_RDMD]

... is worth it when it's simple enough to type

rdmd_test --rdmd [PATH_TO_RDMD]

... especially bearing in mind that the makefile design is set up to avoid anyone having to run rdmd_test directly.

marler8997 · 2018-03-18T20:36:55Z

I've found that dealing with @WebDrake is a very large time sink and has never resulted in improvements. If anyone thinks he brought up any valid points let me know, otherwise I'm going to assume no one has legitimate concerns with this.

Waiting for this to be merged so I can go back to working on rdmd with -i.

CyberShadow · 2018-03-18T20:45:35Z

I don't have the bandwidth to look in-depth into this, nor do I think this is something that we should spend too much time and energy on, but from a quick glance:

5 changes in one commit. These should be at least 5 commits, possibly spread through several PRs. It is much easier to review, argue about, and vet minimal self-contained changes.
Unless they bring an obvious improvement in some real-life use case, changes in the command-line syntax of a test utility are changes with little benefit.

marler8997 · 2018-03-18T20:48:44Z

I definitely don't want to waste time on this either. These are all obvious improvements making rdmd_test much simpler. @WebDrake really messed up the interface with his recent changes. I can split this up into 5 small commits but I don't see any benefit, they are each only on a few lines.

CyberShadow · 2018-03-18T20:51:26Z

Sorry, I can't approve this in its current state.

I also really don't like the idea of pulling information out of help text. It is non-prescriptive.

marler8997 · 2018-03-18T20:51:52Z

The test already pulls information out of the help text

CyberShadow · 2018-03-18T20:58:19Z

The test already pulls information out of the help text

Where? I see code that tests the help string, which is different from pulling out and using information from the help text.

@WebDrake really messed up the interface with his recent changes.

Why do you think so? Anyway, it doesn't really matter because rdmd_test's command line is an implementation detail. make test is now the canonical way to invoke rdmd_test.

I can split this up into 5 small commits but I don't see any benefit, they are each only on a few lines.

The aggregate diff right now is an unreviewable mess IMO. Reading top to bottom, correlating adjacent hunks is too difficult and there is no obvious way to segment the changes into fragments that could be argued about on their own.

marler8997 · 2018-03-18T21:00:48Z

Ok, when I get a chance I"ll split this up int 5 PRs...so much for not wasting time. I'd much rather work on other PRs but if you want me to waste time there's nothing I can do.

WebDrake · 2018-03-18T21:13:18Z

I've found that dealing with @WebDrake is a very large time sink and has never resulted in improvements.

If you would like to describe problems you are having with the current rdmd_test setup I am happy to listen, and to try to propose ways to deal with them.

However, please note that I took a reasonable amount of my own time to give you very detailed feedback. It would be considerate of you to try to take it on board (even if you don't want to actually reply or have a discussion about it).

marler8997 · 2018-03-18T21:18:54Z

@WebDrake like I said, discussing with you is a time sink that adds no value. I just don't want to waste my time.

WebDrake · 2018-03-18T21:25:14Z

The test already pulls information out of the help text

Where exactly ... ? The only case I can find is where it explicitly tests the rdmd --help option and confirms the first line of output (which is reasonable enough).

Note that this is very different from the fallback test, where it matters to verify that the fallback compiler is the one we expect it to be.

Is there any case other than that where --help output is used?

marler8997 · 2018-03-18T21:26:51Z

@WebDrake it is testing the output of the help text...this is "using" the help text. Let's not waste time on this please.

WebDrake · 2018-03-18T21:30:54Z

@WebDrake it is testing the output of the help text...this is "using" the help text. Let's not waste time on this please.

That is very different from your proposed change. I've given you the reasons why.

You might waste less of your time if you would engage constructively with negative feedback. It's offered to try to help you.

marler8997 · 2018-03-18T21:31:50Z

@WebDrake like I said, discussing with you is a time sink that adds no value. I just don't want to waste my time.

marler8997 · 2018-03-19T03:11:45Z

This PR has been split into the following:

#333
#334
#335
#337
#338
#339

ibuclaw · 2018-03-19T19:50:42Z

@marler8997 - Moderate yourself, before I have to.

marler8997 · 2018-03-19T20:42:34Z

@ibuclaw, just trying to be honest. Dealing with @WebDrake had been nothing but a waste of time. What did I say that I should not have?

andralex · 2018-03-19T21:07:26Z

HI folks, this is a bit brusque regardless of the shared history (of which I'm not aware). For however fleeting/fragmented quanta of time, please model interaction with one another after a relationship with a coworker on the same project. So imagine you have nearby desks, bump into each other on hallways, etc. Would make sweeping statements quite a bit more awkward wouldn't it. Small matters should stay small and the response to them proportional. To the extent there are disagreements of a somewhat fundamental nature that prevent working together, please consider me the HR person to whom you'd email a concern. No reply needed, please let's reset and improve. Thanks!

marler8997 requested review from CyberShadow, andralex and wilzbach as code owners March 16, 2018 18:53

marler8997 force-pushed the fixRdmdTest branch from 72d2df3 to 45bca24 Compare March 17, 2018 01:01

marler8997 force-pushed the fixRdmdTest branch from 45bca24 to 2b5ef8e Compare March 17, 2018 01:16

WebDrake reviewed Mar 18, 2018

View reviewed changes

marler8997 closed this Mar 21, 2018

marler8997 deleted the fixRdmdTest branch March 21, 2018 04:48

Uh oh!

Comments

Conversation

marler8997 commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Mar 16, 2018

Bugzilla references

Uh oh!

CyberShadow commented Mar 17, 2018

Uh oh!

WebDrake left a comment

Choose a reason for hiding this comment

Uh oh!

WebDrake Mar 18, 2018

Choose a reason for hiding this comment

Uh oh!

WebDrake Mar 18, 2018

Choose a reason for hiding this comment

Uh oh!

WebDrake Mar 18, 2018

Choose a reason for hiding this comment

Uh oh!

WebDrake Mar 18, 2018

Choose a reason for hiding this comment

Uh oh!

WebDrake Mar 18, 2018

Choose a reason for hiding this comment

Uh oh!

WebDrake Mar 18, 2018

Choose a reason for hiding this comment

Uh oh!

marler8997 commented Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyberShadow commented Mar 18, 2018

Uh oh!

marler8997 commented Mar 18, 2018

Uh oh!

CyberShadow commented Mar 18, 2018

Uh oh!

marler8997 commented Mar 18, 2018

Uh oh!

CyberShadow commented Mar 18, 2018

Uh oh!

marler8997 commented Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WebDrake commented Mar 18, 2018

Uh oh!

marler8997 commented Mar 18, 2018

Uh oh!

WebDrake commented Mar 18, 2018

Uh oh!

marler8997 commented Mar 18, 2018

Uh oh!

WebDrake commented Mar 18, 2018

Uh oh!

marler8997 commented Mar 18, 2018

Uh oh!

marler8997 commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibuclaw commented Mar 19, 2018

Uh oh!

marler8997 commented Mar 19, 2018

Uh oh!

andralex commented Mar 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marler8997 commented Mar 16, 2018 •

edited

Loading

marler8997 commented Mar 18, 2018 •

edited

Loading

marler8997 commented Mar 18, 2018 •

edited

Loading

marler8997 commented Mar 19, 2018 •

edited

Loading