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

Trim console reporter output (#431) #432

Merged
merged 3 commits into from Aug 24, 2020

Conversation

marc136
Copy link
Contributor

@marc136 marc136 commented Aug 6, 2020

Hello, this is my take on reducing the console output to a more manageable amount as discussed in #431

Now it returns a short reproduction command like:
Running 407 tests. To reproduce these results, run: elm-test --fuzz 100 --seed 115440003747078
or
Running 224 tests. To reproduce these results, run: elm-test --fuzz 100 --seed 41574277908519 tests\One.elm tests\Group\*
instead of printing many many lines of absolute file paths.


I changed the data that is passed to all the reporters. I'm confident that it does not break anything for the console reporter, as it only printed the list of files before this commit, and now it prints the list of file globs passed to elm-test.

I'm not sure if it might break something for another tool that works with the output of the json reporter (e.g. if it needs the list of absolute paths of every found test file).

I would really appreciate if this (or a similar change) would make it into this nice tool.

@harrysarson
Copy link
Collaborator

Thanks @marc136, this looks great! However, I am uneasy about the changes to the json and junit reporter though. I am unsure exactly what stability guarantees we make but I think this change to the json/junit output could break things for people.

My thoughts are this:

  1. Keep threading the "paths" value in the same way we currently do (i.e. revert changes to this line) but also thread through a new value called globs. Then we can use the globs value in the console reporter and keep using the paths value in the json. [1]

  2. Delay this change until the next major release of the test runner (i.e. when elm 0.19.2 gets released).

What do you think?


[1]: even better would be to include the globs value aswell as the paths value in the json we output as I think the globs is probably more useful. Moreover, including both would let us also to option (2) at a later date.

@marc136
Copy link
Contributor Author

marc136 commented Aug 7, 2020

Hello @harrysarson,

for my change, I started from the test reporters and then followed the variables up to the point where I made the changes.
The paths value is only used insided reportBegin of all reporters (link to code)
The junit reporter does not use the paths value and the json reporter prints it.

Regarding 1 (and [1]):
I like that proposal, and I think the change could come in any future revision. So this would mean:

  • The globs value is threaded along through all parts (together with paths) and added to the RunInfo record
  • Test.Reporter.Console.reportBegin prints the globs in the reproduction command (instead of all files)
  • Test.Reporter.Json.reportBegin prints the globs as an additional json field
  • Test.Reporter.Junit.reportBegin is not changed and keeps returning Nothing

Regarding 2:
I think it would be good to mark the paths value as deprecated for consumers of the json reporter, but I saw no obvious way to do that and then remove it with the next major release.
We could ask in discourse and slack if anyone is aware of a tool that uses the paths value of the json reporter.

Maybe we can create a new issue for that like "Remove the paths value from the JSON reporter" and list all known tools that use the json report so they could be checked before the change?

@harrysarson harrysarson mentioned this pull request Aug 7, 2020
@harrysarson
Copy link
Collaborator

Sounds like a good plan!

Changes:
The console reporter prints the arguments passed to elm-test and the
`--seed` and `--fuzz` flags. Before, it would list the full path of
every single test file found using the passed file selection glob.

The JSON reporter returns the additional field "globs".
@marc136 marc136 changed the title Pass file selection glob instead of every found file to reporters (#431) Trim console reporter output (#431) Aug 7, 2020
@marc136
Copy link
Contributor Author

marc136 commented Aug 7, 2020

I changed the PR to act as described in my last comment, @harrysarson.

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.

Thanks! I will let this sit for maybe a week berfore merging incase another folk wish to review but this looks really good to me.

@harrysarson harrysarson merged commit 7269fd6 into rtfeldman:master Aug 24, 2020
@harrysarson
Copy link
Collaborator

Thanks @marc136!

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

Successfully merging this pull request may close these issues.

None yet

2 participants