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

Question about the behavior related to duplicate descriptions error and automatic description level added with the module name? #493

Open
mpizenberg opened this issue Feb 9, 2021 · 4 comments

Comments

@mpizenberg
Copy link
Contributor

When the error for duplicate descriptions was introduced, it was in part to get rid of potential copy-paste descriptions for more useful failure messages.

In node-test-runner, all tests are aggregated with their module name prepended to each test description. As a result, potential copy pasted descriptions across test files are not detected. In elm-test-rs, we do not prepend the module name to the description, causing behavior differences such as tests failing in elm-test-rs with duplicate descriptions errors. Although annoying to fix, which would be improved with this PR, @avh4 acknowledged that "In all cases, the duplicate describe names were a mistake".

I must admit that initially my reason for this behavior change was not to catch duplicate descriptions. It was for two reasons, (1) because I didn't like having labels in the failure reports that were never set by the user themselves, especially knowing that it's easy to add a suite = describe "This.Module" [ allExposedTests... ], and (2) because it resulted in simpler code in the tests runner.

Now knowing that it has the following advantages:

  • be explicit about your tests descriptions
  • potentially catch copy-paste errors
  • simpler test runner code

would node-test-runner be willing to change that behavior at the next breaking update? Or are there other advantages than copy-paste commodity that I didn't saw making it worth staying as is?

@lydell
Copy link
Collaborator

lydell commented Feb 9, 2021

node-test-runner’s current behavior also has an edge case quirk: If you try to name a test the same as its module you get a somewhat confusing error.

module TestsPassing exposing (myTest)

import Expect
import Test exposing (test)


myTest =
    test "TestsPassing" <|
        \() ->
                Expect.equal 1 1
The test 'TestsPassing' contains a child test of the same name. Let's rename them so we know which is which.

@razzeee
Copy link

razzeee commented Sep 23, 2021

I actually need to know the module name. So if this gets removed, it would be nice to get a new field with that information. As we can also deduct the filepath from it.

@harrysarson
Copy link
Collaborator

@mpizenberg did the module name get removed?

@mpizenberg
Copy link
Contributor Author

mpizenberg commented Sep 24, 2021

@harrysarson no, not in elm-test, just that we are trying integration of elm-test-rs in the language server, which is extracting the module name from the root label of a description. Thus @razzeee comment.

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

No branches or pull requests

4 participants