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

JSON comparison should ignore LF in comparisons #270

Open
bedantaguru opened this issue Jul 31, 2019 · 6 comments
Open

JSON comparison should ignore LF in comparisons #270

bedantaguru opened this issue Jul 31, 2019 · 6 comments
Labels
bug an unexpected problem or unintended behavior diffs 🔎

Comments

@bedantaguru
Copy link

Thanks a lot for this wonderful package.

I have used this to test a few shiny modules developed in my package. I have noticed a thing about json comparison which is described below

Problem : If I record the test and then push the same in GitHub
I'm getting messages like

warning: LF will be replaced by CRLF in <app test path>/001.json.
The file will have its original line endings in your working directory

Full log here

The tests are passed locally.
Then I clone the same GitHub project in another folder now the test fails

Here is a reproducible example

# internet is required for this

td <- tempfile("test_dir_")

require(git2r)
#> Loading required package: git2r

clone("https://github.com/bedantaguru/tidycells_nightly", local_path = td)
#> cloning into 'C:\Users\igayen\AppData\Local\Temp\RtmpOwptsw\test_dir_fc01d91999'...
#> Receiving objects:   1% (5/409),   63 kb
#> Receiving objects:  11% (45/409), 1408 kb
#> Receiving objects:  21% (86/409), 3370 kb
#> Receiving objects:  31% (127/409), 3426 kb
#> Receiving objects:  41% (168/409), 3538 kb
#> Receiving objects:  51% (209/409), 3538 kb
#> Receiving objects:  61% (250/409), 4154 kb
#> Receiving objects:  71% (291/409), 4154 kb
#> Receiving objects:  81% (332/409), 4378 kb
#> Receiving objects:  91% (373/409), 6732 kb
#> Receiving objects: 100% (409/409), 6866 kb, done.
#> Local:    master C:/Users/igayen/AppData/Local/Temp/RtmpOwptsw/test_dir_fc01d91999
#> Remote:   master @ origin (https://github.com/bedantaguru/tidycells_nightly)
#> Head:     [9184c53] 2019-07-31: shinytest github pull push test

setwd(td)

require(shinytest)
#> Loading required package: shinytest
#> Warning: package 'shinytest' was built under R version 3.6.1
#  you need below package (shiny modules are built on this package)
# https://github.com/r-rudra/tidycells 
# devtools::install_github("r-rudra/tidycells")
require(tidycells)
#> Loading required package: tidycells

# this test is designed for different purpose
unlink("00_nightly_only/shiny_test/exapp/tests/fails_after_close.R")

testApp("00_nightly_only/shiny_test/exapp")
#> Running
#> crop.R 
#> ==== Comparing crop... 
#>   Differences detected between crop-current/ and crop-expected/:
#> 
#>     Name         Status      
#>     001.json  != Files differ
#>     001.png      No change   
#>     002.json  != Files differ
#>     002.png      No change   
#>     003.json  != Files differ
#>     003.png      No change   
#> 
#> To view a textual diff, run:
#>   viewTestDiff("00_nightly_only/shiny_test/exapp", interactive = FALSE)

unlink("00_nightly_only/shiny_test/exapp", recursive = T, force = T)

#  this is exactly same file folder before GitHub push 
unzip("00_nightly_only/shiny_test/exapp.zip", exdir = "00_nightly_only/shiny_test")

# this test is designed for different purpose
unlink("00_nightly_only/shiny_test/exapp/tests/fails_after_close.R")

testApp("00_nightly_only/shiny_test/exapp")
#> Running crop.R 
#> ==== Comparing crop... No changes.

# cleanup
unlink(td, recursive = T)

Created on 2019-07-31 by the reprex package (v0.3.0)

@bedantaguru
Copy link
Author

As per this SO1 and SO2

The solution to this problem is

git config --global core.autocrlf false

But, can we not try reading the json and compare values instead of the exact file comparison?

@hadley hadley added the bug an unexpected problem or unintended behavior label Aug 7, 2020
@hadley hadley changed the title GitHub Pull and Push is causing issues in json which is breaking the test JSON comparison should ignore LF in comparisons Aug 7, 2020
@hadley
Copy link
Member

hadley commented Aug 13, 2020

Easiest solution for shinytest is to use brio, replacing existing calls to write_ut8() and readLines(). Needs to wait on #336 so it's easy to test that the change actually works.

hadley added a commit that referenced this issue Aug 13, 2020
@hadley
Copy link
Member

hadley commented Aug 13, 2020

brio isn't a viable solution because it always adds a trailing newline when writing. This is probably desired behaviour, but it's change from the current writer, so would invalidate all existing snapshots.

@hadley
Copy link
Member

hadley commented Aug 27, 2020

This probably requires someone with a windows computer to figure out; I've given up (see my attempts in the attached PR). Alternatively, I think a better long term approach would be to read the JSON back into R and perform the comparisons with waldo.

@Millchmaedchen
Copy link

Are there any news on this?
I am developing an app in a package and am using gitlab ci mainly based on a windows ci server, windows docker image and windows runner.
I have put the auto crlf to false, however my tests still fail.
I cannot find a diff when manually inspecting the test artifacts (they both have \n as eol), also the check output gives me no diff:

== Failed tests ================================================================
-- Failure (test-app-function.R:60:1): (code run outside of `test_that()`) -----
Not all shinytest scripts passed for apps/my_app_name: full_run_register, login_screen, select_run

Diff output:
==== full_run_register ====

==== login_screen ====

==== select_run ====

If this is expected, use `snapshotUpdate('apps/my_app_name')` to update

While at the same time reporting a diff in every single json:

Running full_run_register.R login_screen.R select_run.R 
==== Comparing full_run_register... 
  Differences detected between full_run_register-current/ and full_run_register-expected/:

    Name         Status      
    001.json  != Files differ
    002.json  != Files differ
    003.json  != Files differ
    004.json  != Files differ
    005.json  != Files differ
    006.json  != Files differ
    007.json  != Files differ
    008.json  != Files differ
    009.json  != Files differ
    010.json  != Files differ
    011.json  != Files differ
==== Comparing login_screen... 
  Differences detected between login_screen-current/ and login_screen-expected/:

    Name         Status      
    001.json  != Files differ
    002.json  != Files differ
==== Comparing select_run... 
  Differences detected between select_run-current/ and select_run-expected/:

    Name         Status      
    001.json  != Files differ
    002.json  != Files differ
    003.json  != Files differ

I am using portable R (4.1.2) and app + dev dependencies from MRAN date 28.02.2022. I am actually developing on windows for windows, however we might have to cover unix as well in the near future.

Thank you for your effort!

@Millchmaedchen
Copy link

I spent some time to figure out, where the supposed diff was coming from and it turns out that fc.exe - the windows diff application - returns the correct exit code (i.e. no difference), while diff.exe from rtools detects some difference at the end of the JSON (I couldn't figure out what it is).
Not sure if this has any other implications, but it might be worthwhile to have fc as the default diff tool on a windows platform. It does the trick for me if I change which_diff() like so:

which_diff <- function() {
  path <- Sys.which("fc")
  if (path == "")
    path <- Sys.which("diff")
  if (path != "")
    return(path)
    
  abort("No program named `diff` or `fc` found in path.")
}

Best,
Milena

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior diffs 🔎
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants