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

gitutils: add diffStrings, diffFiles, and use it in testament to compare expected vs gotten #17892

Merged
merged 12 commits into from
Apr 30, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Apr 29, 2021

  • add diffStrings, diffFiles (can be customized in future with options if needed)
  • replaces all instances of diff -uNdr with an API call
  • testament now uses diffStrings to compare expected vs gotten, making it easier to see what differs (in particular for whitespace issues, but not limited to that)
  • expose tempfiles.genTempPath

example

I added (and reverted) a temporary commit that added intentional changes in test "expected output", to show what it looks like in CI, see https://gist.github.com/timotheecour/515dde92cb29ce359a3f4b9546c247fd

note: unrelated, preexisting bug in std/tempfiles

as shown in https://gist.github.com/timotheecour/515dde92cb29ce359a3f4b9546c247fd, it looks like randomPathName is buggy on windows: see #17898;
this doesn't affect this PR though because I'm using a prefix diffStrings_a and diffStrings_b so there is no clash here (but it's still a bug that should be fixed in future work)

lib/std/tempfiles.nim Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Apr 29, 2021

We could revive experimental/diff.nim for this.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 30, 2021

PTAL

We could revive experimental/diff.nim for this.

this can be done in future work and would be a 2 line fix once experimental/diff gains the required features for this:

  • it doesn't have a human readable output yet, we could in future add toPretty to render it.
  • note also that git diff has more options (eg --word-diff, --color-moved etc) and honors a users' git config to customize how it's rendered (eg with colors etc)

Currently, a primary use case for experimental/diff over git diff is when shelling out is not an option (eg because it needs to be called at high frequency etc). This is not the case here.

@timotheecour timotheecour marked this pull request as ready for review April 30, 2021 01:49
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Apr 30, 2021
@@ -307,7 +308,7 @@ proc addResult(r: var TResults, test: TTest, target: TTarget,
maybeStyledEcho styleBright, expected, "\n"
Copy link
Member Author

Choose a reason for hiding this comment

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

showing expected is still useful (even with showing the diff) so users can copy paste it
(and showing the given is also useful for understanding the output)

@Araq Araq merged commit 20248a6 into nim-lang:devel Apr 30, 2021
@timotheecour timotheecour deleted the pr_diff_api branch April 30, 2021 09:19
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
…are expected vs gotten (nim-lang#17892)

* gitutils: add diffStrings, diffFiles, and use it in testament to compare expected vs gotten
* refactor with createTempDir
* cleanup
* refacotr
* PRTEMP fake test spec changes to show effect of diffStrings
* add runnableExamples for experimental/diff + cross-reference with gitutils
* Revert "PRTEMP fake test spec changes to show effect of diffStrings"

This reverts commit 57dc8d6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants