Skip to content

Introduce golden testing#152

Merged
Ailrun merged 2 commits into
haskell:masterfrom
Ailrun:introduce-golden-testing
Jun 10, 2020
Merged

Introduce golden testing#152
Ailrun merged 2 commits into
haskell:masterfrom
Ailrun:introduce-golden-testing

Conversation

@Ailrun

@Ailrun Ailrun commented Jun 9, 2020

Copy link
Copy Markdown
Member

This PR introduces a golden testing facility and restores most of the format tests.

Related comments: #146 (comment)

@alanz, I want to join this project team. Is it possible to join?

@alanz alanz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

Are we in favour of this as a concept? I think I am.

@alanz alanz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Meant to hit the approve button, sorry.

@jneira

jneira commented Jun 9, 2020

Copy link
Copy Markdown
Member

I am in favor too for some cases, formatting could be one of them.
However, one caveat is string files are platform dependent (at least new lines). I guess those tests pass in linux but not sure if it will do in windows (an its "lovely" \r\n\)
If you use unicode the fun could be endless (i hope it will be not the case)

@jneira
jneira self-requested a review June 9, 2020 22:40
@Ailrun

Ailrun commented Jun 9, 2020

Copy link
Copy Markdown
Member Author

@jneira Hm, so documentContents can start to use \r\n even when the initial file is written in \n? I can imagine it depends on both how formatters and HLS work, but still, sound depressing... Anyhow, that case I will add some kinds of normalization before the saves begin.

@jneira

jneira commented Jun 9, 2020

Copy link
Copy Markdown
Member

@Ailrun let me checkout the branch and run them to confirm it, i hope maybe i am wrong 😄

@Ailrun

Ailrun commented Jun 10, 2020

Copy link
Copy Markdown
Member Author

OK, I will wait your confirmation then.

@jneira

jneira commented Jun 10, 2020

Copy link
Copy Markdown
Member

Well, tests are passing cause the crlf conversion is already being done, a pleasant surprise:

stylish-haskell
      formats a document:                                                 warning: LF will be replaced by CRLF in C:\TEMP\StyB51A.actual.
The file will have its original line endings in your working directory
OK (21.80s)
      formats a range:                                                    warning: LF will be replaced by CRLF in C:\TEMP\StyC6FE.actual.
The file will have its original line endings in your working directory
OK (20.56s)
    brittany
      formats a document with LF endings:                                 warning: LF will be replaced by CRLF in C:\TEMP\BriE360.actual.
The file will have its original line endings in your working directory
OK (19.45s)
      formats a document with CRLF endings:                               warning: LF will be replaced by CRLF in C:\TEMP\BriF3FB.actual.
The file will have its original line endings in your working directory
OK (21.09s)
      formats a range with LF endings:                                    warning: LF will be replaced by CRLF in C:\TEMP\Bri60D.actual.
The file will have its original line endings in your working directory
OK (20.53s)
      formats a range with CRLF endings:                                  warning: LF will be replaced by CRLF in C:\TEMP\Bri12C0.actual.
The file will have its original line endings in your working directory
OK (19.20s)
    ormolu
      formats correctly:                                                  warning: LF will be replaced by CRLF in C:\TEMP\For1D31.actual.
The file will have its original line endings in your working directory
OK (14.64s)

🎉

@Ailrun

Ailrun commented Jun 10, 2020

Copy link
Copy Markdown
Member Author

Great! Let me merge this since it works in Windows too :)

@Ailrun
Ailrun merged commit 2186df0 into haskell:master Jun 10, 2020
@jneira

jneira commented Jun 10, 2020

Copy link
Copy Markdown
Member

I wonder where the conversion is being done, maybe tasty-golden?

@Ailrun

Ailrun commented Jun 10, 2020

Copy link
Copy Markdown
Member Author

@jneira afaik, they don't handle such a conversion. I just googled those error messages and I found they are from git, not from any of Haskell codes. (FYI, the tests use git diff to print diffed outputs)

@Ailrun
Ailrun deleted the introduce-golden-testing branch June 10, 2020 16:58
@jneira

jneira commented Jun 10, 2020

Copy link
Copy Markdown
Member

oh, yeah, the message is exactly that one, i dint know it was using git diff, the las time i used it it asked for the diff utility and i had to install it in windows.
But i am afraid that tests were succesful cause i have auto.crlf git option activated and maybe they might fail with that option disabled. I will try

@Ailrun

Ailrun commented Jun 10, 2020

Copy link
Copy Markdown
Member Author

FYI, the tasty-golden does not force users to use a specific tool to compute differences between a golden file and test result (even though their documentation mentions diff), and I believe git diff is more widespread, so I choose that to get the diff result.

auto.crlf might be the case, and if tests do not work without it, I will send a PR for a normalizer of document contents.

@jneira

jneira commented Jun 10, 2020

Copy link
Copy Markdown
Member

thanks for the info, git diff is a nice alternative.
setting auto.crlf to false did not make the tests fail so no need for more changes

@Ailrun

Ailrun commented Jun 10, 2020

Copy link
Copy Markdown
Member Author

Great, thank you for checking Windows cases!

pepeiborra pushed a commit that referenced this pull request Dec 27, 2020
This fixes some issues where we used an uppercase drive letter in the
import path even though the LSP client uses lowercase drive letters
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.

3 participants