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

Improve elm-test.js #465

Merged
merged 39 commits into from
Nov 14, 2020
Merged

Conversation

lydell
Copy link
Collaborator

@lydell lydell commented Oct 30, 2020

Fixes #211
Addresses #442 (comment)

This PR makes a bunch of improvements to elm-test.js:

  • Properly validate CLI arguments:

    • Error on unknown flags.
    • Validate flag values early (seed and fuzz must be numbers, report must be console/json/junit).
    • Default flag values early, instead of in code generation or even in Elm.
  • elm-test.js is now 100% definitions, except the last line which is main(); to kick things off. Previously, it was difficult to understand in what order things executed. There was some top-level code, a bunch of function definitions, some more top-level code, more functions, and so on. Now, you can follow the call stack from main(). Also, this means that there are way fewer global variables – functions now get passed what they depend on.

  • Use async/await where it helps (in preparation for merge conflicts with Moving to async/await: lib/Runner.js #464 – it should be possible to just pick this PR’s code for every conflict in elm-test.js).

  • Improved help message. Also, add some aliases, such as -h, for showing help. I find it super annoying when CLI tools make it difficult to find the help – which is the key to all other commands and flags.

  • Various cleanups.

Help before:

Usage: elm-test init # Create example tests

Usage: elm-test install PACKAGE # Like `elm install PACKAGE`, except it installs to "test-dependencies" in your elm.json

Usage: elm-test TESTFILES # Run TESTFILES, for example tests/**/*.elm

Usage: elm-test [--compiler /path/to/compiler] # Run tests

Usage: elm-test [--seed integer] # Run with initial fuzzer seed

Usage: elm-test [--fuzz integer] # Run with each fuzz test performing this many iterations

Usage: elm-test [--report json, junit, or console (default)] # Print results to stdout in given format

Usage: elm-test [--version] # Print version string and exit

Usage: elm-test [--watch] # Run tests on file changes

Intermediate help (outdated):

elm-test init
    Create example tests

elm-test
    Run tests in the tests/ folder

elm-test TESTFILES
    Run TESTFILES, for example "src/**/*Tests.elm"

elm-test install PACKAGE
    Like `elm install PACKAGE`, except it installs to
    "test-dependencies" in your elm.json

Options:
    --compiler /path/to/compiler
        Use a custom path to an Elm executable. Default: elm
    --seed INT
        Run with a previous fuzzer seed. Default: A random seed
    --fuzz INT
        Run with each fuzz test performing this many iterations. Default: 100
    --report json
    --report junit
    --report console
        Print results to stdout in the given format. Default: console
    --version
        Print version and exit
    --watch
        Run tests on file changes

commander help:

Usage: elm-test [options] [globs...]

elm-test
  Run tests in the tests/ folder

elm-test "src/**/*Tests.elm"
  Run tests in files matching the glob

Options:
  --compiler <path>              Use a custom path to an Elm executable (default: elm)
  --seed <int>                   Run with a previous fuzzer seed (default: 68917302767402)
  --fuzz <int>                   Run with each fuzz test performing this many iterations (default: 100)
  --report <json|junit|console>  Print results to stdout in the given format (default: "console")
  --watch                        Run tests on file changes (default: false)
  --version                      Print version and exit
  -h, --help                     Show help

Commands:
  init                           Create example tests
  install <package>              Like `elm install package`, except it installs to "test-dependencies" in your elm.json
  make [globs...]                Check files matching the globs for compilation errors
  help [command]                 Show help

@harrysarson harrysarson added the status: misc in progress We are actively working on this pull request. label Oct 31, 2020
@lydell
Copy link
Collaborator Author

lydell commented Nov 1, 2020

@harrysarson I think I’ve addressed all comments now!

@harrysarson harrysarson added status: waiting for review Pull request needs a review and removed status: misc in progress We are actively working on this pull request. labels Nov 1, 2020
lib/Flags.js Outdated Show resolved Hide resolved
lib/Flags.js Outdated
// Or the next argument: `--flag value`.
const getValue = (fallback /*: string */) /*: string */ => {
if (match === null) {
index++;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not like that we have a for loop and then a closure that mutates the index variable. My lunch break is almost over though and I haven't come up with a more constructive suggestion yet. Let me go and think about this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We talked about this on discord. @lydell is going to look into some alternatives to minimist

@harrysarson harrysarson added status: waiting for author Review comments need addressing. and removed status: waiting for review Pull request needs a review labels Nov 4, 2020
@harrysarson harrysarson added status: waiting for review Pull request needs a review and removed status: waiting for author Review comments need addressing. labels Nov 4, 2020
lib/elm-test.js Show resolved Hide resolved
lib/elm-test.js Show resolved Hide resolved
lib/elm-test.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@harrysarson harrysarson left a comment

Choose a reason for hiding this comment

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

Just a couple of nits. I like the flag parsing with commander and I think you have done a nice job of hacking around/dealing with its shortcomings such that is is clear what is going on.

Have you tested these changes against any project test suites?

lib/elm-test.js Show resolved Hide resolved
lib/elm-test.js Outdated Show resolved Hide resolved
lib/elm-test.js Show resolved Hide resolved
lib/elm-test.js Show resolved Hide resolved
lib/elm-test.js Show resolved Hide resolved
lib/elm-test.js Outdated Show resolved Hide resolved
@harrysarson harrysarson added status: waiting for author Review comments need addressing. and removed status: waiting for review Pull request needs a review labels Nov 9, 2020
@lydell
Copy link
Collaborator Author

lydell commented Nov 9, 2020

Have you tested these changes against any project test suites?

I’m not sure what to test?

@harrysarson
Copy link
Collaborator

I was just thinking

cd /some/ideally/complex/project
/path/to/test/runner/bin/elm-test 

as a sanity check.

@lydell
Copy link
Collaborator Author

lydell commented Nov 10, 2020

Ok! I tried that at work and in the elm-review repo and it worked fine. --watch worked too.

@harrysarson harrysarson added status: waiting for review Pull request needs a review and removed status: waiting for author Review comments need addressing. labels Nov 10, 2020
@harrysarson harrysarson merged commit 2e2fd09 into rtfeldman:master Nov 14, 2020
@lydell lydell deleted the improve-elm-test.js branch November 14, 2020 22:25
harrysarson pushed a commit that referenced this pull request Nov 17, 2020
Regression from #465.

Before:

```
❯ ../bin/elm-test install truqu/elm-md5
I am having trouble with this argument:

    t

It is supposed to be a <package> value, like one of these:

    elm/html
    elm/http
    elm/svg
    elm/time
```

After:

```
❯ ../bin/elm-test install truqu/elm-md5
Here is my plan:

  Add:
    truqu/elm-md5            1.1.0
    zwilias/elm-utf-tools    2.0.1

Would you like me to update your elm.json accordingly? [Y/n]:
Success!
```

This happened because I thought the first argument of the `.action` callback is always an array. Wrong! The signature depends on the `.command` syntax provided:

```js
program
  .command('name <one> <two> [three] [rest...]')
  .action((one: string, two: string, three: string | undefined, rest: Array<string>, cmd: Command) => {
    // Do stuff
  });
```

For the install command we have:

```js
program
  .command('install <package>')
  .action((packageName: string, cmd: Command) => {})
```

Before we tried to destructure the first argument (thinking it was an array), so we only got the first letter of the package name.

Unfortunately our test suite doesn’t catch this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for review Pull request needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-determinism with --seed argument Command line flags aren't validated
2 participants