rnp icon indicating copy to clipboard operation
rnp copied to clipboard

CLI tests: drop a strange behaviour of file_text() utility function

Open andrey-utkin opened this issue 2 years ago • 2 comments

file_text() is a function in src/tests/cli_common.py which reads the whole file (in binary mode, so it doesn't transform newlines in OS-dependent way) and returns it as a Python string, making a two-liner task a single function call.

But it also does .replace('\r\r', '\r') on the resulting string, which is questionable for a function which has a very general-purpose name, doesn't have this behaviour documented, and is used in 36 places across the test suite. It's unclear what was the original intent, and whether this was a series of typos or not (\r\n -> \n?).

Dropping this transformation altogether doesn't affect any of existing tests.

andrey-utkin avatar Jun 21 '22 15:06 andrey-utkin

Codecov Report

Merging #1860 (5c445af) into master (49726d3) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1860   +/-   ##
=======================================
  Coverage   81.53%   81.53%           
=======================================
  Files         138      138           
  Lines       28873    28873           
=======================================
  Hits        23542    23542           
  Misses       5331     5331           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 49726d3...5c445af. Read the comment docs.

codecov[bot] avatar Jun 21 '22 15:06 codecov[bot]

This pull request introduces 1 alert when merging 5c445afd715dc3ecabc2c9bae3b6f0f1ea75e139 into 49726d350885ae4710c52e99884c7e95063d904e - view on LGTM.com

new alerts:

  • 1 for Imprecise assert

lgtm-com[bot] avatar Jun 21 '22 16:06 lgtm-com[bot]

Let's close this as doesn't seem relevant/doesn't solve anything at the moment.

ni4 avatar Nov 21 '22 14:11 ni4