Skip to content

Conversation

@evantypanski
Copy link
Contributor

@evantypanski evantypanski commented Apr 22, 2025

  1. Makes btest consider more than just the current directory when looking for the config file - it will now look in parent directories until it finds it
  2. Allows relative paths in the absence of a valid test
  3. Allows running the tests from outside of the test suite (eg btest zeek/testing/btest/language/while.zeek)

There are a couple edge cases I'll check tomorrow-or-so, therefore this is a draft. And the failing test is a little sad (where it finds the parent's btest config even though it should fail), may need a different config file name so that it doesn't clash with the test suite's btest config.

@evantypanski evantypanski force-pushed the topic/etyp/recurse-cfg-upwards branch from 97092cc to 3c754ac Compare April 23, 2025 13:45
@evantypanski evantypanski marked this pull request as ready for review April 23, 2025 13:57
@evantypanski evantypanski force-pushed the topic/etyp/recurse-cfg-upwards branch from 3c754ac to 61bffc2 Compare April 23, 2025 14:48
@evantypanski evantypanski requested a review from awelzel April 23, 2025 14:49
@evantypanski evantypanski force-pushed the topic/etyp/recurse-cfg-upwards branch from 61bffc2 to a83c459 Compare April 23, 2025 15:32
@evantypanski
Copy link
Contributor Author

I updated it to make sure dots work from relative directories, but that specifies the test exactly. So the dot syntax should stay the same, where you can't specify a "relative" test with dots.

I think the edge cases are mostly covered too, dots seem to work the same and I think temp directories don't change since they cd to the test base anyway. It just changes 2 places files are found.

@evantypanski evantypanski force-pushed the topic/etyp/recurse-cfg-upwards branch from a83c459 to 1eb4b45 Compare April 23, 2025 17:06
@evantypanski evantypanski force-pushed the topic/etyp/recurse-cfg-upwards branch 3 times, most recently from d0dbd14 to bf71a3e Compare April 24, 2025 18:42
Copy link
Contributor

@awelzel awelzel left a comment

Choose a reason for hiding this comment

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

Suggest to get another approval from @bbannier or @rsmmr.

Comments are mostly nitpickery, though it'd be nice to have the special case for the test base directory imo.

@evantypanski evantypanski force-pushed the topic/etyp/recurse-cfg-upwards branch from d332fe2 to ba6f941 Compare April 25, 2025 15:26
@evantypanski evantypanski requested a review from rsmmr April 25, 2025 15:26
@evantypanski evantypanski force-pushed the topic/etyp/recurse-cfg-upwards branch 3 times, most recently from 1680970 to ff7cd9f Compare April 25, 2025 19:25
Copy link
Member

@rsmmr rsmmr left a comment

Choose a reason for hiding this comment

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

There are cases where we don't abort if not finding a btest.cfg:

# pwd
/Users/robin/work/zeek/master/auxil/btest/testing
# ../btest /etc
/etc/manpaths: mandatory keyword '@TEST-EXEC' or '@TEST-EXEC-FAIL' is missing..

One question: Once we have found btest.cfg, is the current working directory then consistently set to where it's located, independent from where btest was called? Is should, and from a quick test, I couldn't find a counter example; but from the code, I wasn't really sure.

@evantypanski evantypanski force-pushed the topic/etyp/recurse-cfg-upwards branch from ff7cd9f to c8ba86c Compare April 28, 2025 12:58
@evantypanski
Copy link
Contributor Author

is the current working directory then consistently set to where it's located, independent from where btest was called?

I believe so, it changes to the directory that the config file was found in. That's (thankfully) why all of the temp files and directories in the config just magically worked with this change. Seems like that's necessary for when a different config file is passed in, even before this change, so the machinery was there.

There are cases where we don't abort if not finding a btest.cfg

Seems like btest also lets you execute ignored files as tests, it just adds them if it doesn't already know about them and you passed a file in. I personally would rather that case just error, but maybe it has some use?

Copy link
Member

@rsmmr rsmmr left a comment

Choose a reason for hiding this comment

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

Ok, so approving, I don't want to hold this up on the rare/impossible case of multiple configs. If you can easily make that one change of finding all configs, I'd still prefer that, otherwise fine as is.

@evantypanski evantypanski force-pushed the topic/etyp/recurse-cfg-upwards branch 2 times, most recently from 3e33a5e to fd0c25c Compare April 29, 2025 14:55
@evantypanski
Copy link
Contributor Author

For the test I had to update the diff-remove-abspath script (or it combined the two paths...). That was roughly from Zeek's (with modification).

This seems to be the consensus behavior, minus style nits

@evantypanski evantypanski requested a review from awelzel April 29, 2025 14:59
If btest.cfg is in the `testing/btest` relative directory, this allows
the user to call btest with `btest testing/btest/tests`, which will use
the btest.cfg file in testing/btest. It will also check its parents, so
you may use `btest testing/btest/language/while.zeek` - for example.

The first complication is that previously-valid uses of btest should
remain the same. This is done by always checking the current directory
for btest.cfg first.

The second is if there is a conflict between two tests' btest.cfg file.
For example, the following may choose two different btest.cfg files:

.
└── multiple-cfg
    ├── Baseline
    ├── btest.cfg
    └── tests
        ├── no-cfg
        │   └── test2.test
        └── with-cfg
            ├── btest.cfg
            └── tests
                └── test1.test

If there is a conflicting config file, then btest should raise an error,
as it can currently only execute with one config file per run.
@evantypanski evantypanski force-pushed the topic/etyp/recurse-cfg-upwards branch from fd0c25c to 28b2f89 Compare April 30, 2025 13:53
@evantypanski evantypanski merged commit 8c0fbfd into master Apr 30, 2025
22 checks passed
@evantypanski evantypanski deleted the topic/etyp/recurse-cfg-upwards branch April 30, 2025 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants