Bulk baseline update to reflect btest 0.64 #1309

ckreibich · 2020-11-24T06:57:51Z

This includes a few canonifier updates to improve the normalized baselines; see the commits for details. It includes bumps of the various submodules with btest baselines, all currently pointing at branches of the same name. These are:

There's also a corresponding branch for the external testsuite (I finally used it, wheee!):

https://github.com/zeek/zeek-testing/tree/topic/christian/gh-1307-baseline-refresh

I've reviewed the updated baselines relatively thoroughly ... I think I caught obvious deficiencies in the canonifiers (see above and existing discussion in #1307 ), but it's of course possible I missed something.

I verified that the testsuites still pass on the updated baselines.

Resolves #1307.

0xxon · 2020-11-27T13:46:24Z

Just to also add my comment from #1307 here - what is the reason for us doing all the baseline updates at once?

We could just do this and slowly move over to the canonified baseline when a baseline changes, right? Which might make it less probable that we miss something.

0xxon · 2020-11-30T13:18:53Z

@ckreibich - there is a whole bunch of tests that currently fails for me on MacOs (10.15) when calling btest manually in the testing directory.

To give an example:

$ ../../auxil/btest/btest -d core.option-errors
[  0%] core.option-errors ... failed
  % 'TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff .stderr' failed unexpectedly (exit code 1)
  % cat .diag
  == File ===============================
  error in /Users/johanna/bro/master/testing/btest/.tmp/core.option-errors/option-errors.zeek, line 4: no type given (testbool)
  == Diff ===============================
  --- /tmp/test-diff.99646..stderr.baseline.tmp	2020-11-30 13:11:21.000000000 +0000
  +++ /tmp/test-diff.99646..stderr.tmp	2020-11-30 13:11:21.000000000 +0000
  @@ -1 +1 @@
  -error in <...>/option-errors.zeek, line 4: no type given (testbool)
  +error in <...>/master/testing/btest/.tmp/core.option-errors/option-errors.zeek, line 4: no type given (testbool)
  =======================================

  % cat .stderr
  error in /Users/johanna/bro/master/testing/btest/.tmp/core.option-errors/option-errors.zeek, line 4: no type given (testbool)

1 of 1 test failed

I am not entirely sure why that happens for me - and not for CI.

0xxon · 2020-11-30T13:26:16Z

This seems to be some disagreement between the different sed versions; when installing (and using) gnu sed it works.

For reference, I am using the sed that comes with Mac Os - which does not carry any version identification whatsoever :)

0xxon · 2020-11-30T13:43:13Z

Ok, that took me a while. \t is not tab on os-x sed, so it matches the letter t. Apparently just inserting a literal tab instead will work; I will try this and then add this to this branch to re-run CI.

0xxon · 2020-11-30T14:03:27Z

Ok, another one. Any files that have no newlines at the end of the line now seem to be broken, at least on my osx installation.

Example again:

$ btest -d bifs.enable_raw_output
[  0%] bifs.enable_raw_output ... failed
  % 'btest-diff output' failed unexpectedly (exit code 1)
  % cat .diag
  == File ===============================
  helloXworldhi== Diff ===============================
  --- /tmp/test-diff.93728.output.baseline.tmp	2020-11-30 13:53:59.000000000 +0000
  +++ /tmp/test-diff.93728.output.tmp	2020-11-30 13:53:59.000000000 +0000
  @@ -1 +1 @@
  -helloXworldhi
  \ No newline at end of file
  +helloXworldhi
  =======================================

  % cat .stderr

1 of 1 test failed

It seems that, at least in this case, MacOs is of the opinion that there is a newline at the end of the output file; linux is of the opinion that there is no newline at the end of output file. Changing it to the other makes the test pass on os-x and fail on Linux.

This one I am a bit more stymified about. There was no newline at the end of the original test-file either. The only difference between the old file and the new file seems to be the addition of the comment line at the beginning - which seems to break the comparison on os-x.

Since no canonifiers are used here, this actually looks like a new bug in btest-diff, when handling the new type of file. Removing the btest-diff-version comment-line also makes the test pass again.

ckreibich · 2020-11-30T16:34:33Z

MacOs is of the opinion that there is a newline at the end of the output file; linux is of the opinion that there is no newline at the end of output file

LOL :-) — thanks for digging into these, Johanna. I'll see if I can find anything.

0xxon · 2020-11-30T17:21:00Z

Ok, getting there slowly. It turns out that testing/scripts/diff-remove-timestamps outputs a newline to non-newline-terminated files on os-x, and not on linux. This, however, is not new behavior and seems to have been like this for a while; it just now suddenly matters since the canonification is no longer done on the same system as the comparison :)

0xxon · 2020-11-30T17:23:36Z

...and in what should surprise no one, this is a difference between os-x sed and GNU sed again :)

0xxon · 2020-12-04T12:44:31Z

Ok, in things that I learned about this. Apparently sed is only supposed to operate on text files - and files that do not have an eol at the end of the file are not seen as text files. So the behavior for this is unspecified.

There seems to be no way to get os-x sed to not emit newlines at the end of the file; the typical suggestion is to use perl instead (which we don't want to do).

I am tempted to just force linux to output a newline at the end of all these tests instead - however, this obviously is also a bit sad, because it would be nice to be able to check that the output files really do not have a newline at the end.

0xxon · 2020-12-04T14:10:15Z

I think I have a solution to the line end problem. So - next one. A bunch of tests fail for me on os-x with the following error - and hang forever:

[ 66%] scripts.base.frameworks.file-analysis.irc ... failed
  % 'btest-diff thefile' failed unexpectedly (exit code 1)
  % cat .diag
Traceback (most recent call last):
  File "/Users/johanna/bro/btest/btest", line 2542, in <module>
    (succeeded, failed, skipped, unstable, failed_expected) = TestManager(address=addr).run(copy.deepcopy(tests), output_handler)
  File "/Users/johanna/bro/btest/btest", line 340, in run
    self.threadRun(0)
  File "/Users/johanna/bro/btest/btest", line 383, in threadRun
    t.run(self)
  File "/Users/johanna/bro/btest/btest", line 1005, in run
    self.mgr.testFailed(self)
  File "/Users/johanna/bro/btest/btest", line 563, in testFailed
    self._output_handler.testFailed(test, msg)
  File "/Users/johanna/bro/btest/btest", line 1336, in testFailed
    h.testFailed(test, msg)
  File "/Users/johanna/bro/btest/btest", line 1654, in testFailed
    self.showDiag(test)
  File "/Users/johanna/bro/btest/btest", line 1628, in showDiag
    for line in open(f):
  File "/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/lib/python3.8/codecs.py", line 322, in decode
    (result, consumed) = self._buffer_decode(data, self.errors, final)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xae in position 50: invalid start byte

0xxon · 2020-12-04T15:37:16Z

Ok, the reason for this one is that we are btest-diff-ing binary files. And btest is not really 100% ready to deal with binary baselines. This specific instance is not hard to fix, however I am not sure this is the only gotcha with this.

That might be me - but I also feel not super happy about having the btest-comment on top of a binary file - that feels a bit weird.

0xxon

I think I fixes the problems that prevented the baselines from passing on OS-X.

Someone else should probably take a look at the changes I performed. I am also not a huge fan about the fact that we currently change binary files to include the btest-canonification-headers in them - like e.g. testing/btest/Baseline/scripts.base.frameworks.file-analysis.http.pipeline/3-file (which is a gif)

Perhaps an option for btest-diff telling it that a file is binary, and to not change it would make sense here.

ckreibich · 2020-12-04T17:05:40Z

the canonification is no longer done on the same system as the comparison :)

I think this is really the key bit here ... and the fact that this might be problematic hadn't occurred to me. I'd also argue that it shouldn't be problematic. Sigh.

Perhaps an option for btest-diff telling it that a file is binary, and to not change it would make sense here.

Yeah, for binary files the whole premise of canonification (hopefully) doesn't apply anyway.

Thanks (again) for digging into these. I'll look through your updates shortly.

ckreibich · 2020-12-05T01:27:26Z

I've started with the binary mode support over in zeek/btest#39 and will now go over this branch assuming it. With it, instead of saying

@TEST-EXEC env -u TEST_DIFF_CANONIFIER btest-diff extract_files/1

you'd just say:

@TEST-EXEC btest-diff --binary extract_files/1

and the baseline would be left alone.

I'll rebase while I'm at it, too.

ckreibich · 2020-12-05T07:42:39Z

This latest push hopefully covers the controversial binary-file cases. It does pass on FreeBSD for me, and I'm now going to test it on Linux.

Actually, I need to qualify the FreeBSD statement: I noticed that the Zeek testsuite is generally unstable for me on FreeBSD when run parallelized, at least on my 4-core VM. Problems happen mostly in cluster-related tests, roughly one of which seems to fail on each run. I'll dig into that further once the dust on the baselines has settled a bit.

(Not rebased yet)

This pattern got mislead by matching suffixes of other numbers, and noramlizing exact 0-timestamps isn't really required. - Remove eplicit "0.000000" number pattern from timestamp normalization - Require beginning of line or non-numeric character before the beginning of the number replacement

This avoids swallowing multiple separate paths separated by unrelated content into one substitution, like here: orig_p=59856<...>/tcp] -> orig_p=59856/tcp, resp_h=192.150.187.43, resp_p=80/tcp]

…-double test This now more surgically applies canonifiers so that the double-format numerical output isn't itself canonified.

\t does not work on OS-X and just matches the letter t. This commit replaces it with a literal tab instead.

By default all baslines are run through diff-remove-timestamp. On a BSD sed implementation, this means that a newline is added to the end of the file, if no newline was there originally. This behavior differs from GNU sed, which does not add a newline. In this commit we unify this behavior by always adding a newline, even when using GNU sed. This commit also disables the canonifier for a bunch of binary baselines, so we do not have to change them.

(Preliminary commit to be updated later)

This converts Johanna's TEST_DIFF_CANONIFIER removals via "env -u" to using the new "btest-diff --binary", and updates the affected baselines.

ckreibich · 2020-12-07T05:55:57Z

Okay, I've now gone through and rebased everything. The branches are the same still as posted in the beginning, with an additional one for the private testsuite, which I'd overlooked earlier (thx Johanna):

https://github.com/zeek/zeek-testing-private/tree/topic/christian/gh-1307-baseline-refresh

Fingers crossed we can merge this time ... 🤞

0xxon · 2020-12-07T09:20:43Z

Ok, I will go through all of this again and try to merge it.

0xxon · 2020-12-07T18:22:11Z

I am going to merge this in a couple of minutes - with the exception of the zeekctl changes. I am still waiting for the baselines to run here and opened zeek/zeekctl#28

In the future it generally would be great if you could open a PR for each submodule that is touched - even if that is a bit annoying it makes all the tests run :)

ckreibich · 2020-12-07T18:36:42Z

Yay, thanks Johanna. 🎉

Yeah ... I'm happy to handle these large updates as you prefer. Maybe the first guideline should be to avoid them when feasible... here, seems we could have gone "bottom-up" with PRs for the submodules first. Is that what you mean? It also always makes me nervous to point submodules at my branches, because the merge master needs to update all of these if we use merge commits.

jsiwek · 2020-12-07T18:49:03Z

It also always makes me nervous to point submodules at my branches, because the merge master needs to update all of these if we use merge commits.

Think it's unavoidable if we want to know how a changeset spanning the full tree of Git submodules behaves under CI before we start merging stuff since it will only get a coherent checkout of the full tree if the submodules are actually changed within the root repo(s) of associated PRs.

jsiwek added Area: CI/Testing Type: Maintenance labels Nov 24, 2020

jsiwek added this to the 4.0.0 milestone Nov 24, 2020

0xxon self-assigned this Nov 30, 2020

0xxon requested changes Dec 4, 2020

View reviewed changes

0xxon removed their assignment Dec 4, 2020

ckreibich mentioned this pull request Dec 5, 2020

Add "binary mode" to btest-diff zeek/btest#39

Merged

ckreibich added 2 commits December 6, 2020 18:19

Make diff-remove-abspath canonifier match on non-whitespace paths only

3984d99

This avoids swallowing multiple separate paths separated by unrelated content into one substitution, like here: orig_p=59856<...>/tcp] -> orig_p=59856/tcp, resp_h=192.150.187.43, resp_p=80/tcp]

ckreibich force-pushed the topic/christian/gh-1307-baseline-refresh branch from e1322a0 to 5368975 Compare December 7, 2020 03:57

ckreibich and others added 4 commits December 6, 2020 20:19

Baseline refresh to reflect btest 0.64

0b674eb

Canonifier improvements for the scripts.base.frameworks.logging.ascii…

c0bd02e

…-double test This now more surgically applies canonifiers so that the double-format numerical output isn't itself canonified.

Bump submodules for btest 0.64 update

f3482ef

Fix diff-remove-abspath on OSX

7040e30

\t does not work on OS-X and just matches the letter t. This commit replaces it with a literal tab instead.

0xxon and others added 5 commits December 6, 2020 20:19

Update external baseline commit hashes

5f6b95c

Switch to btest-diff with --binary support

c5fd520

(Preliminary commit to be updated later)

Update btest-diff calls on binary files to using "--binary"

17cd740

This converts Johanna's TEST_DIFF_CANONIFIER removals via "env -u" to using the new "btest-diff --binary", and updates the affected baselines.

Additional use of btest-diff --binary

b04082c

ckreibich force-pushed the topic/christian/gh-1307-baseline-refresh branch from 5368975 to b04082c Compare December 7, 2020 04:20

0xxon self-assigned this Dec 7, 2020

0xxon merged commit c85d6d6 into master Dec 7, 2020

ckreibich deleted the topic/christian/gh-1307-baseline-refresh branch December 15, 2020 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bulk baseline update to reflect btest 0.64 #1309

Bulk baseline update to reflect btest 0.64 #1309

ckreibich commented Nov 24, 2020

0xxon commented Nov 27, 2020

0xxon commented Nov 30, 2020

0xxon commented Nov 30, 2020 •

edited

Loading

0xxon commented Nov 30, 2020

0xxon commented Nov 30, 2020 •

edited

Loading

ckreibich commented Nov 30, 2020

0xxon commented Nov 30, 2020 •

edited

Loading

0xxon commented Nov 30, 2020

0xxon commented Dec 4, 2020

0xxon commented Dec 4, 2020

0xxon commented Dec 4, 2020

0xxon left a comment

ckreibich commented Dec 4, 2020

ckreibich commented Dec 5, 2020

ckreibich commented Dec 5, 2020 •

edited

Loading

ckreibich commented Dec 7, 2020

0xxon commented Dec 7, 2020

0xxon commented Dec 7, 2020

ckreibich commented Dec 7, 2020

jsiwek commented Dec 7, 2020

Bulk baseline update to reflect btest 0.64 #1309

Bulk baseline update to reflect btest 0.64 #1309

Conversation

ckreibich commented Nov 24, 2020

0xxon commented Nov 27, 2020

0xxon commented Nov 30, 2020

0xxon commented Nov 30, 2020 • edited Loading

0xxon commented Nov 30, 2020

0xxon commented Nov 30, 2020 • edited Loading

ckreibich commented Nov 30, 2020

0xxon commented Nov 30, 2020 • edited Loading

0xxon commented Nov 30, 2020

0xxon commented Dec 4, 2020

0xxon commented Dec 4, 2020

0xxon commented Dec 4, 2020

0xxon left a comment

Choose a reason for hiding this comment

ckreibich commented Dec 4, 2020

ckreibich commented Dec 5, 2020

ckreibich commented Dec 5, 2020 • edited Loading

ckreibich commented Dec 7, 2020

0xxon commented Dec 7, 2020

0xxon commented Dec 7, 2020

ckreibich commented Dec 7, 2020

jsiwek commented Dec 7, 2020

0xxon commented Nov 30, 2020 •

edited

Loading

0xxon commented Nov 30, 2020 •

edited

Loading

0xxon commented Nov 30, 2020 •

edited

Loading

ckreibich commented Dec 5, 2020 •

edited

Loading