test: replaced hard coded folders with os.path.join & replaced os.path.sep #16896

renepickhardt commented at 11:33 AM on September 17, 2019: none

This should fix the firstGoodIssue #16894 (which I discovered with carldongs firstGoodIssueTwitter bot c.f.: https://twitter.com/GoodFirstIssues/status/1173915080439795714)

have no windows machine here and couldn't test. relying on ci on github.

replaced hard coded folders with os.path.join and replaced os.path.sep in regex to fix the firstGoodIssue #16894 (which I discovered with carldongs firstGoodIssueTwitter bot) a539bc67d3

MarcoFalke commented at 11:37 AM on September 17, 2019: member

Would be nice to leave the whitespace changes for later. It makes it impossible to review the commit that fixes a bug.

fanquake added the label Tests on Sep 17, 2019

mzumsande commented at 11:43 AM on September 17, 2019: member

Thanks for picking this up! If everything succeeds, combine_logs won't be invoked. So for testing, you could add a second commit that makes some functional test trivially fail and after checking the CIs' output remove it again and add a link to the logs here.

renepickhardt commented at 11:47 AM on September 17, 2019: none

Would be nice to leave the whitespace changes for later. It makes it impossible to review the commit that fixes a bug.

sorry my emacs just has some auto save actions to follow the PEP8 formatting. I didn't even realize that that has happened.

What do you (@MarcoFalke ) want me to do? I could quickly mark the 4 relevant lines here on github or do you want me to redo the patch without changes to whitespace?

in test/functional/combine_logs.py:87 in a539bc67d3

  82 | @@ -78,16 +83,21 @@ def read_logs(tmp_dir):
  83 |      for each of the input log files."""
  84 |  
  85 |      # Find out what the folder is called that holds the debug.log file
  86 | -    chain = glob.glob("{}/node0/*/debug.log".format(tmp_dir))
  87 | +
  88 | +    chain = glob.glob(os.path.join(tmp_dir, "node0", "*", "debug.log"))

renepickhardt commented at 11:47 AM on September 17, 2019:

first change

in test/functional/combine_logs.py:93 in a539bc67d3

  91 | -        chain = re.search(r'node0/(.+?)/debug\.log$', chain).group(1)  # extract the chain name
  92 | +        # pick the first one if more than one chain was found (should never happen)
  93 | +        chain = chain[0]
  94 | +        sep = os.path.sep
  95 | +        chain = re.search(r'node0' + sep + '(.+?)' + sep +
  96 | +                          'debug\.log$', chain).group(1)  # extract the chain name

renepickhardt commented at 11:47 AM on September 17, 2019:

second change

practicalswift commented at 9:02 PM on September 26, 2019:

Nit: \. is not a valid escape sequence :)

in test/functional/combine_logs.py:98 in a539bc67d3

  98 | -        chain = 'regtest'  # fallback to regtest (should only happen when none exists)
  99 | +        # fallback to regtest (should only happen when none exists)
 100 | +        chain = 'regtest'
 101 |  
 102 | -    files = [("test", "%s/test_framework.log" % tmp_dir)]
 103 | +    files = [("test", os.path.join(tmp_dir, "test_framework.log"))]

renepickhardt commented at 11:48 AM on September 17, 2019:

third change

in test/functional/combine_logs.py:100 in a539bc67d3

 101 |  
 102 | -    files = [("test", "%s/test_framework.log" % tmp_dir)]
 103 | +    files = [("test", os.path.join(tmp_dir, "test_framework.log"))]
 104 |      for i in itertools.count():
 105 | -        logfile = "{}/node{}/{}/debug.log".format(tmp_dir, i, chain)
 106 | +        logfile = os.path.join(tmp_dir, "node{}".format(i), chain, "debug.log")

renepickhardt commented at 11:48 AM on September 17, 2019:

fourth change

in test/functional/combine_logs.py:114 in a539bc67d3

 110 | @@ -101,18 +111,21 @@ def print_node_warnings(tmp_dir, colors):
 111 |      warnings = []
 112 |      for stream in ['stdout', 'stderr']:
 113 |          for i in itertools.count():
 114 | -            folder = "{}/node{}/{}".format(tmp_dir, i, stream)
 115 | +            folder = os.path.join(tmp_dir, "node{}".format(i), stream)

renepickhardt commented at 11:49 AM on September 17, 2019:

5th change

in test/functional/combine_logs.py:120 in a539bc67d3

 117 |                  break
 118 |              for (_, _, fns) in os.walk(folder):
 119 |                  for fn in fns:
 120 | -                    warning = pathlib.Path('{}/{}'.format(folder, fn)).read_text().strip()
 121 | +                    warning = pathlib.Path(os.path.join(
 122 | +                        folder, fn)).read_text().strip()

renepickhardt commented at 11:49 AM on September 17, 2019:

6th change

renepickhardt commented at 11:50 AM on September 17, 2019: none

actually the whitespace stuff didn't mess it up so badly everything between line 81 and 120 that was changed is actually a change that would have occured even without the whitespace formatting changes. everything above and blow can easily be neglected.

MarcoFalke commented at 11:59 AM on September 17, 2019: member

We don't enforce a maximum line length, and different contributors have different preferences about the line length. Thus, we can't (for now) have editors auto-format a file that was saved. Otherwise, the lines would just ping-pong around on every change. Generally, we leave the line length like it was previously.

DrahtBot commented at 3:22 PM on September 17, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#8994 (Testchains: Introduce custom chain whose constructor... by jtimon)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke commented at 4:35 PM on September 17, 2019: member

Also, I think it would be easier to read the commit message if it was shorter. See https://chris.beams.io/posts/git-commit/

mzumsande commented at 4:59 PM on September 17, 2019: member

This doesn't seem to be working yet. I cherry-picked your commit in order to understand why AppVeyor doesn't like my PR #16756 (that's the background why I created #16894), and combine_logs.py still doesn't work: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/27470906 Also, the linter complains about an invalid escaping sequence. https://travis-ci.org/bitcoin/bitcoin/jobs/586103029

renepickhardt commented at 5:47 PM on September 17, 2019: none

@mzumsande I am a little bit confused about the error. First of all the error message changed which (hopefully means progress) but I cannot make much of the error message as I don't see where unbalanced parenthesis would come from. Also I am somewhat confused that at least in the error message there is still the usage of / and ... @MarcoFalke once @mzumsande get the code running I will make a force push with the patch withouth outsave actions and with a better commit message

in test/functional/combine_logs.py:92 in a539bc67d3

  90 | -        chain = chain[0]  # pick the first one if more than one chain was found (should never happen)
  91 | -        chain = re.search(r'node0/(.+?)/debug\.log$', chain).group(1)  # extract the chain name
  92 | +        # pick the first one if more than one chain was found (should never happen)
  93 | +        chain = chain[0]
  94 | +        sep = os.path.sep
  95 | +        chain = re.search(r'node0' + sep + '(.+?)' + sep +

mzumsande commented at 10:01 PM on September 17, 2019:

I think the issue is that the windows separator \ needs to be escaped when used in a regex. Not a great expert on regex, but with the following it should work:

if sep == '\\':
            sep = '\\\\'

jamesob commented at 2:25 PM on September 18, 2019:

Would prefer we just use pathlib here.

mzumsande changes_requested

mzumsande commented at 10:09 PM on September 17, 2019: member

See my post below. Also see my suggestion in post 3, if all functional tests pass, you can't see if your change works, as the current AppVeyor build shows.

laanwj commented at 12:28 PM on September 18, 2019: member

Thanks for contributing. Concept ACK.

I'd also suggest removing the whitespace and formatting changes (at least to lines not touched otherwise) and focusing the changes on the issue that you're trying to fix. This makes reviewing easier, and keeps the git commit log more readable.

fanquake renamed this:
~~replaced hard coded folders with os.path.join & replaced os.path.sep~~
test: replaced hard coded folders with os.path.join & replaced os.path.sep
on Sep 18, 2019

promag commented at 1:48 PM on September 18, 2019: member

Agree with @laanwj, limit the change to the PR scope.

jamesob changes_requested

MarcoFalke commented at 7:05 PM on September 18, 2019: member

Yeah, pretty sure this is just a trivial one line fix. No need to blow up the scope of this pull request beside the minimal required fix.

MarcoFalke added the label Waiting for author on Sep 18, 2019

MarcoFalke added the label Up for grabs on Sep 25, 2019

mzumsande commented at 9:51 PM on September 26, 2019: member

Since this has gone inactive and marked "up for grabs", I opened #16973.

MarcoFalke closed this on Sep 26, 2019

MarcoFalke removed the label Up for grabs on Sep 26, 2019

MarcoFalke referenced this in commit d5a770b70d on Oct 10, 2019

sidhujag referenced this in commit ef6d2956e9 on Oct 11, 2019

laanwj removed the label Waiting for author on Oct 24, 2019

Munkybooty referenced this in commit f7624c6ca7 on Oct 7, 2021

Munkybooty referenced this in commit 7a3163e7a1 on Oct 14, 2021

Munkybooty referenced this in commit daf0ebcd5e on Oct 16, 2021

MarcoFalke locked this on Dec 16, 2021