Repository: RasaHQ/rasa
Pull request: for "rasa test core --evaluate-model-directory" set default for "--model" to directory
**Proposed changes**:
If rasa test core is called with --evaluate-model-directory and a file as --model, use the folder that file is in as --model instead.

This fixes #4896 : When no --model argument is given, instead of defaulting to the latest model in ./models it now defaults to ./models

Side effect: When called with "--model some_folder/some_file.tar.gz", it uses some_folder as --model instead. Is this something we want? If yes, should the code throw a warning like "you provided a file as --model, will use the folder that file is in instead"?

**Status (please check what you already did)**:
- [x] added some tests for the functionality
- [ ] updated the documentation
- [x] updated the changelog (please check [changelog](https://github.com/RasaHQ/rasa/tree/master/changelog) for instructions)
- [x] reformat files using `black` (please check [Readme](https://github.com/RasaHQ/rasa#code-style) for instructions)


Problem statement:
"rasa test core --evaluate-model-directory" should have directory as default
**Steps to Reproduce**:
1. Hava a rasa project (e.g. by doing `rasa init`)
2. Train a single model (`rasa train`)
3. Run `rasa test core --evaluate-model-directory`

**Problem**:

By default `rasa test core` has the latest model in the `./models` directory as default value for the `--model` parameter.

This can lead to the following error:
In case there is a regular model in the `models` directory, this will be assumed as latest model.  However, `rasa test core --evaluate-model-directory` expects a directory and will fail with an error like this:

```
Traceback (most recent call last):
  File "/Users/tobias/.pyenv/versions/3.7/bin/rasa", line 11, in <module>
    load_entry_point('rasa', 'console_scripts', 'rasa')()
  File "/Users/tobias/Workspace/stack/rasa/__main__.py", line 76, in main
    cmdline_arguments.func(cmdline_arguments)
  File "/Users/tobias/Workspace/stack/rasa/cli/test.py", line 83, in test_core
    test_core_models_in_directory(model_path, stories, output)
  File "/Users/tobias/Workspace/stack/rasa/test.py", line 26, in test_core_models_in_directory
    number_of_stories = io_utils.read_json_file(story_n_path)
  File "/Users/tobias/Workspace/stack/rasa/utils/io.py", line 144, in read_json_file
    content = read_file(filename)
  File "/Users/tobias/Workspace/stack/rasa/utils/io.py", line 136, in read_file
    with open(filename, encoding=encoding) as f:
NotADirectoryError: [Errno 20] Not a directory: 'models/20191204-102617.tar.gz/num_stories.json'
```

**Proposed Solutions**

This can by automatically taking the directory instead of the file in case the user used the `--evaluate-model-directory` flag.


**Other things to check and potentially create a new issue for it**:
In case the models directory is empty (except the `run` folders from the comparison training) the default value for `--models` is `None`. So we have to actively check if there is a `None` value in the passed models list.

Hints from issue discussion:

Commit message: split up tests in 3 functions; more descriptive name; added missing types

Patch to review:
diff --git a/rasa/test.py b/rasa/test.py
index d62a14b5e23f..95045c011402 100644
--- a/rasa/test.py
+++ b/rasa/test.py
@@ -2,6 +2,7 @@
 import logging
 import os
 from typing import Text, Dict, Optional, List, Any
+from pathlib import Path
 
 import rasa.utils.io as io_utils
 from rasa.constants import (
@@ -16,9 +17,13 @@
 logger = logging.getLogger(__name__)
 
 
-def test_core_models_in_directory(model_directory: Text, stories: Text, output: Text):
+def test_core_models_in_directory(
+    model_directory: Text, stories: Text, output: Text
+) -> None:
     from rasa.core.test import compare_models_in_dir, plot_core_results
 
+    model_directory = _test_core_models_in_directory_input(model_directory)
+
     loop = asyncio.get_event_loop()
     loop.run_until_complete(compare_models_in_dir(model_directory, stories, output))
 
@@ -27,6 +32,20 @@ def test_core_models_in_directory(model_directory: Text, stories: Text, output:
     plot_core_results(output, number_of_stories)
 
 
+def _test_core_models_in_directory_input(model_directory: Text) -> Text:
+    import rasa.model
+
+    p = Path(model_directory)
+    if p.is_file():
+        if model_directory != rasa.model.get_latest_model():
+            print_warning(
+                "You passed a file as '--model'. Will use the directory containing this file instead."
+            )
+        model_directory = str(p.parent)
+
+    return model_directory
+
+
 def test_core_models(models: List[Text], stories: Text, output: Text):
     from rasa.core.test import compare_models
 
diff --git a/tests/test_test.py b/tests/test_test.py
new file mode 100644
index 000000000000..1c8a05f620c4
--- /dev/null
+++ b/tests/test_test.py
@@ -0,0 +1,78 @@
+from pathlib import Path
+from _pytest.capture import CaptureFixture
+from _pytest.monkeypatch import MonkeyPatch
+
+import rasa.model
+import rasa.cli.utils
+
+
+def monkeypatch_get_latest_model(tmp_path: Path, monkeypatch: MonkeyPatch):
+
+    latest_model = tmp_path / "my_test_model.tar.gz"
+
+    def mock_get_latest_model():
+        return str(latest_model)
+
+    monkeypatch.setattr(rasa.model, "get_latest_model", mock_get_latest_model)
+
+
+def test_test_core_models_in_directory_input_default(
+    capsys: CaptureFixture, tmp_path: Path, monkeypatch: MonkeyPatch
+):
+    from rasa.test import _test_core_models_in_directory_input
+
+    monkeypatch_get_latest_model(tmp_path, monkeypatch)
+
+    latest_model = Path(rasa.model.get_latest_model())
+    latest_model.touch()
+
+    # Input: default model file
+    # => Should return containing directory
+    modeldir = rasa.model.get_latest_model()
+    p = Path(modeldir)
+    new_modeldir = _test_core_models_in_directory_input(modeldir)
+    captured = capsys.readouterr()
+    assert captured.out == ""
+    assert new_modeldir == str(p.parent)
+
+
+def test_test_core_models_in_directory_input_file(
+    capsys: CaptureFixture, tmp_path: Path, monkeypatch: MonkeyPatch
+):
+    from rasa.test import _test_core_models_in_directory_input
+
+    monkeypatch_get_latest_model(tmp_path, monkeypatch)
+
+    other_model = tmp_path / "my_test_model1.tar.gz"
+    assert str(other_model) != rasa.model.get_latest_model()
+    other_model.touch()
+
+    # Input: some file
+    # => Should return containing directory and print a warning
+    modeldir = str(other_model)
+    p = Path(modeldir)
+    new_modeldir = _test_core_models_in_directory_input(modeldir)
+    captured = capsys.readouterr()
+    rasa.cli.utils.print_warning(
+        "You passed a file as '--model'. Will use the directory containing this file instead."
+    )
+    warning = capsys.readouterr()
+    assert captured.out == warning.out
+    assert new_modeldir == str(p.parent)
+
+
+def test_test_core_models_in_directory_input_other(
+    capsys: CaptureFixture, tmp_path: Path, monkeypatch: MonkeyPatch
+):
+    from rasa.test import _test_core_models_in_directory_input
+
+    monkeypatch_get_latest_model(tmp_path, monkeypatch)
+
+    # Input: anything that is not an existing file
+    # => Should return input
+    modeldir = "random_dir"
+    assert not Path(modeldir).is_file()
+    new_modeldir = _test_core_models_in_directory_input(modeldir)
+    captured = capsys.readouterr()
+    assert captured.out == ""
+    assert new_modeldir == modeldir


Code review comments:
diff_hunk: @@ -27,6 +32,20 @@ def test_core_models_in_directory(model_directory: Text, stories: Text, output:
     plot_core_results(output, number_of_stories)
 
 
+def _test_core_models_in_directory_input(model_directory: Text) -> Text:
+    import rasa.model
+
+    p = Path(model_directory)
path: rasa/test.py
text: @user1:
I'd say this is optional because the function is very short, but in general it makes sense to give variables meaningful names, e.g.
```suggestion
    path_to_model_directory = Path(model_directory)
```

In this case it's really an edge case to be fair.

diff_hunk: @@ -0,0 +1,78 @@
+from pathlib import Path
+from _pytest.capture import CaptureFixture
+from _pytest.monkeypatch import MonkeyPatch
+
+import rasa.model
+import rasa.cli.utils
+
+
+def monkeypatch_get_latest_model(tmp_path: Path, monkeypatch: MonkeyPatch):
+
+    latest_model = tmp_path / "my_test_model.tar.gz"
+
+    def mock_get_latest_model():
+        return str(latest_model)
+
+    monkeypatch.setattr(rasa.model, "get_latest_model", mock_get_latest_model)
+
+
+def test_test_core_models_in_directory_input_default(
+    capsys: CaptureFixture, tmp_path: Path, monkeypatch: MonkeyPatch
+):
+    from rasa.test import _test_core_models_in_directory_input
+
+    monkeypatch_get_latest_model(tmp_path, monkeypatch)
+
+    latest_model = Path(rasa.model.get_latest_model())
+    latest_model.touch()
+
+    # Input: default model file
+    # => Should return containing directory
+    modeldir = rasa.model.get_latest_model()
+    p = Path(modeldir)
+    new_modeldir = _test_core_models_in_directory_input(modeldir)
+    captured = capsys.readouterr()
+    assert captured.out == ""
+    assert new_modeldir == str(p.parent)
+
+
+def test_test_core_models_in_directory_input_file(
path: tests/test_test.py
text: @user1:
assuming you're renaming the function `_test_core_models_in_directory` I'd do this:

```suggestion
def test_get_sanitized_model_directory_when_passing_model_file_explicitly(
```

diff_hunk: @@ -0,0 +1,78 @@
+from pathlib import Path
+from _pytest.capture import CaptureFixture
+from _pytest.monkeypatch import MonkeyPatch
+
+import rasa.model
+import rasa.cli.utils
+
+
+def monkeypatch_get_latest_model(tmp_path: Path, monkeypatch: MonkeyPatch):
+
+    latest_model = tmp_path / "my_test_model.tar.gz"
+
+    def mock_get_latest_model():
+        return str(latest_model)
+
+    monkeypatch.setattr(rasa.model, "get_latest_model", mock_get_latest_model)
+
+
+def test_test_core_models_in_directory_input_default(
+    capsys: CaptureFixture, tmp_path: Path, monkeypatch: MonkeyPatch
+):
+    from rasa.test import _test_core_models_in_directory_input
+
+    monkeypatch_get_latest_model(tmp_path, monkeypatch)
+
+    latest_model = Path(rasa.model.get_latest_model())
+    latest_model.touch()
+
+    # Input: default model file
+    # => Should return containing directory
+    modeldir = rasa.model.get_latest_model()
+    p = Path(modeldir)
+    new_modeldir = _test_core_models_in_directory_input(modeldir)
+    captured = capsys.readouterr()
+    assert captured.out == ""
+    assert new_modeldir == str(p.parent)
+
+
+def test_test_core_models_in_directory_input_file(
+    capsys: CaptureFixture, tmp_path: Path, monkeypatch: MonkeyPatch
+):
+    from rasa.test import _test_core_models_in_directory_input
+
+    monkeypatch_get_latest_model(tmp_path, monkeypatch)
+
+    other_model = tmp_path / "my_test_model1.tar.gz"
+    assert str(other_model) != rasa.model.get_latest_model()
+    other_model.touch()
+
+    # Input: some file
+    # => Should return containing directory and print a warning
+    modeldir = str(other_model)
+    p = Path(modeldir)
+    new_modeldir = _test_core_models_in_directory_input(modeldir)
+    captured = capsys.readouterr()
+    rasa.cli.utils.print_warning(
+        "You passed a file as '--model'. Will use the directory containing this file instead."
path: tests/test_test.py
text: @user1:
Copying over the `print_warning(...)` statement does not provide any value over copying the printed `string` itself. I'd actually just test if there was anything printed at all instead of testing for the specifics because then you don't have to adapt the test just because somebody changes the warning slightly..

@author:
The `print_warning(...)` function adds some symbols to the beginning and end of the `string` before printing - that's what makes it coloured. So I can't just test if the `string` was printed.
But sure, just testing if anything was printed is easier, will do that.

diff_hunk: @@ -0,0 +1,78 @@
+from pathlib import Path
+from _pytest.capture import CaptureFixture
+from _pytest.monkeypatch import MonkeyPatch
+
+import rasa.model
+import rasa.cli.utils
+
+
+def monkeypatch_get_latest_model(tmp_path: Path, monkeypatch: MonkeyPatch):
+
+    latest_model = tmp_path / "my_test_model.tar.gz"
+
+    def mock_get_latest_model():
+        return str(latest_model)
+
+    monkeypatch.setattr(rasa.model, "get_latest_model", mock_get_latest_model)
path: tests/test_test.py
text: @user1:
optional (whatever you prefer)
```suggestion
    monkeypatch.setattr(rasa.model, "get_latest_model", lambda: str(latest_model))
```

diff_hunk: @@ -0,0 +1,78 @@
+from pathlib import Path
+from _pytest.capture import CaptureFixture
+from _pytest.monkeypatch import MonkeyPatch
+
+import rasa.model
+import rasa.cli.utils
+
+
+def monkeypatch_get_latest_model(tmp_path: Path, monkeypatch: MonkeyPatch):
path: tests/test_test.py
text: @user1:
```suggestion
def monkeypatch_get_latest_model(tmp_path: Path, monkeypatch: MonkeyPatch) -> None:
```

diff_hunk: @@ -0,0 +1,78 @@
+from pathlib import Path
+from _pytest.capture import CaptureFixture
+from _pytest.monkeypatch import MonkeyPatch
+
+import rasa.model
+import rasa.cli.utils
+
+
+def monkeypatch_get_latest_model(tmp_path: Path, monkeypatch: MonkeyPatch):
+
+    latest_model = tmp_path / "my_test_model.tar.gz"
+
+    def mock_get_latest_model():
path: tests/test_test.py
text: @user1:
```suggestion
    def mock_get_latest_model() -> Text:
```

diff_hunk: @@ -0,0 +1,78 @@
+from pathlib import Path
+from _pytest.capture import CaptureFixture
+from _pytest.monkeypatch import MonkeyPatch
+
+import rasa.model
+import rasa.cli.utils
+
+
+def monkeypatch_get_latest_model(tmp_path: Path, monkeypatch: MonkeyPatch):
+
+    latest_model = tmp_path / "my_test_model.tar.gz"
+
+    def mock_get_latest_model():
+        return str(latest_model)
+
+    monkeypatch.setattr(rasa.model, "get_latest_model", mock_get_latest_model)
+
+
+def test_test_core_models_in_directory_input_default(
+    capsys: CaptureFixture, tmp_path: Path, monkeypatch: MonkeyPatch
+):
+    from rasa.test import _test_core_models_in_directory_input
+
+    monkeypatch_get_latest_model(tmp_path, monkeypatch)
+
+    latest_model = Path(rasa.model.get_latest_model())
+    latest_model.touch()
+
+    # Input: default model file
+    # => Should return containing directory
+    modeldir = rasa.model.get_latest_model()
+    p = Path(modeldir)
+    new_modeldir = _test_core_models_in_directory_input(modeldir)
+    captured = capsys.readouterr()
+    assert captured.out == ""
+    assert new_modeldir == str(p.parent)
path: tests/test_test.py
text: @user1:
```suggestion
    assert new_modeldir == str(tmp_path)
```

@author:
I don't want to hard-code this in case the `get_latest_model` mock function ever gets changed to return a different path. Using `latest_model.parent` should work though. So I can still get rid of the unnecessary `p` variable.

diff_hunk: @@ -0,0 +1,78 @@
+from pathlib import Path
+from _pytest.capture import CaptureFixture
+from _pytest.monkeypatch import MonkeyPatch
+
+import rasa.model
+import rasa.cli.utils
+
+
+def monkeypatch_get_latest_model(tmp_path: Path, monkeypatch: MonkeyPatch):
+
+    latest_model = tmp_path / "my_test_model.tar.gz"
+
+    def mock_get_latest_model():
+        return str(latest_model)
+
+    monkeypatch.setattr(rasa.model, "get_latest_model", mock_get_latest_model)
+
+
+def test_test_core_models_in_directory_input_default(
+    capsys: CaptureFixture, tmp_path: Path, monkeypatch: MonkeyPatch
+):
+    from rasa.test import _test_core_models_in_directory_input
+
+    monkeypatch_get_latest_model(tmp_path, monkeypatch)
+
+    latest_model = Path(rasa.model.get_latest_model())
+    latest_model.touch()
+
+    # Input: default model file
+    # => Should return containing directory
+    modeldir = rasa.model.get_latest_model()
+    p = Path(modeldir)
+    new_modeldir = _test_core_models_in_directory_input(modeldir)
+    captured = capsys.readouterr()
+    assert captured.out == ""
path: tests/test_test.py
text: @user1:
optional: a bit slicker way to check it:
```suggestion
    assert not captured.out
```

diff_hunk: @@ -0,0 +1,78 @@
+from pathlib import Path
+from _pytest.capture import CaptureFixture
+from _pytest.monkeypatch import MonkeyPatch
+
+import rasa.model
+import rasa.cli.utils
+
+
+def monkeypatch_get_latest_model(tmp_path: Path, monkeypatch: MonkeyPatch):
+
+    latest_model = tmp_path / "my_test_model.tar.gz"
+
+    def mock_get_latest_model():
+        return str(latest_model)
+
+    monkeypatch.setattr(rasa.model, "get_latest_model", mock_get_latest_model)
+
+
+def test_test_core_models_in_directory_input_default(
+    capsys: CaptureFixture, tmp_path: Path, monkeypatch: MonkeyPatch
+):
+    from rasa.test import _test_core_models_in_directory_input
+
+    monkeypatch_get_latest_model(tmp_path, monkeypatch)
+
+    latest_model = Path(rasa.model.get_latest_model())
+    latest_model.touch()
+
+    # Input: default model file
+    # => Should return containing directory
+    modeldir = rasa.model.get_latest_model()
+    p = Path(modeldir)
+    new_modeldir = _test_core_models_in_directory_input(modeldir)
path: tests/test_test.py
text: @user1:
```suggestion
    new_modeldir = _test_core_models_in_directory_input(str(latest_model))
```

diff_hunk: @@ -16,9 +17,13 @@
 logger = logging.getLogger(__name__)
 
 
-def test_core_models_in_directory(model_directory: Text, stories: Text, output: Text):
+def test_core_models_in_directory(
+    model_directory: Text, stories: Text, output: Text
+) -> None:
     from rasa.core.test import compare_models_in_dir, plot_core_results
 
+    model_directory = _test_core_models_in_directory_input(model_directory)
path: rasa/test.py
text: @user1:
`_test_core_models_in_directory_input` doesn't really tell what the helper function is doing / what it is used for.

How about (just a suggestion / feel free to find a better name 😀):
```suggestion
    model_directory = _get_sanitized_model_directory(model_directory)
```