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

Introduce golden testing #152

Merged
merged 2 commits into from
Jun 10, 2020
Merged

Conversation

Ailrun
Copy link
Member

@Ailrun Ailrun commented Jun 9, 2020

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?

Copy link
Collaborator

@alanz alanz left a comment

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.

Copy link
Collaborator

@alanz alanz left a comment

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
Copy link
Member

jneira commented Jun 9, 2020

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
Copy link
Member Author

Ailrun commented Jun 9, 2020

@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
Copy link
Member

jneira commented Jun 9, 2020

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

@Ailrun
Copy link
Member Author

Ailrun commented Jun 10, 2020

OK, I will wait your confirmation then.

@jneira
Copy link
Member

jneira commented Jun 10, 2020

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
Copy link
Member Author

Ailrun commented Jun 10, 2020

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

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

jneira commented Jun 10, 2020

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

@Ailrun
Copy link
Member Author

Ailrun commented Jun 10, 2020

@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
Copy link
Member

jneira commented Jun 10, 2020

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
Copy link
Member Author

Ailrun commented Jun 10, 2020

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
Copy link
Member

jneira commented Jun 10, 2020

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
Copy link
Member Author

Ailrun commented Jun 10, 2020

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