Opened 6 weeks ago

Closed 4 weeks ago

#31637 closed enhancement (fixed)

Make sure we have test coverage for Option, +Option and /Option across defaults, torrc, command line

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: network-team-roadmap-august asn-merge
Cc: nickm, teor, gaba Actual Points: 1.5
Parent ID: #29211 Points:
Reviewer: teor Sponsor:

Description

The tor man page says:

By default, an option on the command line overrides an option found in the configuration file, and an option in a configuration file overrides one in the defaults file.


This rule is simple for options that take a single value, but it can become complicated for options that are allowed to occur more than once: if you specify four SocksPorts in your configuration file, and one more SocksPort on the command line, the option on the command line will replace all of the SocksPorts in the configuration file. If this isn’t what you want, prefix the option name with a plus sign (+), and it will be appended to the previous set of options instead. For example, setting SocksPort 9100 will use only port 9100, but setting +SocksPort 9100 will use ports 9100 and 9050 (because this is the default).


Alternatively, you might want to remove every instance of an option in the configuration file, and not replace it at all: you might want to say on the command line that you want no SocksPorts at all. To do that, prefix the option name with a forward slash (/). You can use the plus sign (+) and the forward slash (/) in the configuration file and on the command line.

Where are our tests for this specification, and do they have the coverage we need to test this refactor?

Child Tickets

Change History (22)

comment:1 Changed 6 weeks ago by teor

Maybe we should also test %include?

comment:2 Changed 6 weeks ago by nickm

FWIW, the current tests for + and / are in test_confparse_reassign_extend(). It would be good to have integration tests too.

comment:3 Changed 6 weeks ago by nickm

There are some tests for %include in src/test/test_config.c, but it would be good to have integration tests there too.

comment:4 Changed 6 weeks ago by nickm

The --dump-config command-line option for Tor will be useful for this.

comment:5 Changed 5 weeks ago by nickm

I've started a shell script that will be useful for this, and maybe also for parts of #31631.

It isn't done yet! I've only written the script and a few example tests for it -- I need to write a bunch more tests before I can call this done, and also I need to write a changes file. But before I more ahead, I'd like to know what you think of this approach. No need to do a full review now: I just want to know whether you think I should proceed with this line of work.

The branch is ticket31637_wip. PR at https://github.com/torproject/tor/pull/1320 .

(WRT #31631: It tests configuration round-trips, but not state or sr_disk_state.)

comment:6 Changed 5 weeks ago by nickm

Reviewer: teor

comment:7 Changed 5 weeks ago by nickm

(I'm currently wrestling with appveyor to try work around CRLF issues. Any advice here would be welcome)

comment:8 Changed 5 weeks ago by nickm

Okay, I've got something that works. See notes above: It's not ready to merge, but I'd like to know how the approach looks to you.

comment:9 Changed 5 weeks ago by nickm

Status: assignedneeds_review

comment:10 Changed 5 weeks ago by teor

Status: needs_reviewnew
Type: defectenhancement

I like this idea.
I also think it could replace some of the repetitive parts of test_options.c

I have some questions about the design:

  1. Should we name the tests, rather than numbering them?
  2. Will we ever have enough tests that we want to split tests into categories, and put each category of tests in its own directory?
  3. Are there any tests that we can't do using this framework? a) We can't test the basic "missing torrc, missing defaults torrc, no command line args" case, because the script supplies an empty file, instead of a missing defaults torrc b) We can't do tests that expect error on some platforms, but success on others (is "Sandbox 1" an example of this kind of test?)

comment:11 Changed 5 weeks ago by teor

Oh I also put some detailed comments on the pull request.

comment:12 in reply to:  10 Changed 5 weeks ago by nickm

Replying to teor:

I like this idea.
I also think it could replace some of the repetitive parts of test_options.c

I have some questions about the design:

  1. Should we name the tests, rather than numbering them?

I think we should.

  1. Will we ever have enough tests that we want to split tests into categories, and put each category of tests in its own directory?

Possibly. I think that should be an easy change to make later, if we decide to do so. We'd just replace a "*" with a "*/*", and do a "git mv" command.

  1. Are there any tests that we can't do using this framework? a) We can't test the basic "missing torrc, missing defaults torrc, no command line args" case, because the script supplies an empty file, instead of a missing defaults torrc

That's right. I think this kind of test would do better in test_cmdline.sh.

b) We can't do tests that expect error on some platforms, but success on others (is "Sandbox 1" an example of this kind of test?)

There are some options like this; I don't think that Sandbox 1 is one of them. (For me, it passes --verify-config on OSX.)

We could extend the script later on if needed to accomodate this -- for example, by having it check for "expected.linux" and "error.osx". But I think I'd like to see how far we get with the first version of the script.

comment:13 Changed 5 weeks ago by nickm

I've messed around with the branch and tried to fix the issues -- then I tried to fix the issues caused by fixing those issues 🙄.

I'm going to start writing a nice big test suite here.

comment:14 Changed 5 weeks ago by nickm

Okay, I did a less big test suite than I had expected here, but I believe it covers +Option and /Option.

I'm not sure how exactly to make it handle %include though -- The problem is that %include relative paths are apparently interpreted relative to Tor's cwd. I think my options are:

  • Defer integration testing for %include directives.
  • Make sure that the test script runs from a known location inside the source directory.
  • Add a step to generate the %include paths in our examples before we run the test scripts.
  • Open a ticket to change how relative paths are interpreted by %include.

I think that the second idea is reasonable for now, but the fourth might be what we want to do long-term. What do you think?

comment:15 in reply to:  14 Changed 5 weeks ago by teor

Replying to nickm:

Okay, I did a less big test suite than I had expected here, but I believe it covers +Option and /Option.

I'm not sure how exactly to make it handle %include though -- The problem is that %include relative paths are apparently interpreted relative to Tor's cwd.

Oh dear. I thought we tried to stop using the cwd for everything.

Maybe we should open a ticket to run tests from an empty temp cwd, so we catch issues like this in future.

I think my options are:

  • Defer integration testing for %include directives.

Adds risk to refactoring, let's try something else first.

  • Make sure that the test script runs from a known location inside the source directory.

Good idea, and probably something we want to do for test stability anyway.

  • Add a step to generate the %include paths in our examples before we run the test scripts.

Yeah let's not unless we have to: unless we get that step exactly right, it could cause instability or failures.

  • Open a ticket to change how relative paths are interpreted by %include.

Yes I think we should do this long-term.

I think that the second idea is reasonable for now, but the fourth might be what we want to do long-term. What do you think?

comment:16 Changed 5 weeks ago by nickm

  • Open a ticket to change how relative paths are interpreted by %include.

Yes I think we should do this long-term.

I've opened #31737 for this.

comment:17 Changed 4 weeks ago by nickm

I've opened a new branch, ticket31637, which squashes all of the work above. PR at https://github.com/torproject/tor/pull/1324 . I expect that the CI will still pass, but you never know.

comment:18 Changed 4 weeks ago by nickm

Actual Points: 1.5
Status: newneeds_review

CI has passed.

comment:19 Changed 4 weeks ago by teor

As an aside, I wonder if we can implement a test for #31408 using this framework?

comment:20 Changed 4 weeks ago by teor

Status: needs_reviewneeds_revision

This looks great - I'm really happy with this level of testing.

It looks like we're testing a lot of non-default options.
Did we try to test them all?

Should we open a ticket for the tricky ones that we couldn't test?
HardwareAccel
ControlSocket
ControlSocketWriteable
Or are we just trying for a decent level of coverage right now?

I'm happy to merge, if these tests also pass on 0.4.1.
(Or if there is a good explanation for why they don't pass, like a new option.)
That way, we can be sure that we aren't just testing the post-refactor behaviour.

Feel free to flip to merge_ready once 0.4.1 passes.

comment:21 in reply to:  20 Changed 4 weeks ago by nickm

Keywords: asn-merge added
Status: needs_revisionmerge_ready

Replying to teor:

This looks great - I'm really happy with this level of testing.

It looks like we're testing a lot of non-default options.
Did we try to test them all?

I tried to avoid testing options, options that don't work on windows, and authority-only options.

Should we open a ticket for the tricky ones that we couldn't test?
HardwareAccel
ControlSocket
ControlSocketWriteable
Or are we just trying for a decent level of coverage right now?

I was just trying for a decent level of coverage here. I've opened #31756 for this.

I'm happy to merge, if these tests also pass on 0.4.1.
(Or if there is a good explanation for why they don't pass, like a new option.)
That way, we can be sure that we aren't just testing the post-refactor behaviour.

Feel free to flip to merge_ready once 0.4.1 passes.

The tests pass for 0.4.1, with one exception: the large_1 test gives its output lines in a different order, since we previously didn't sort the options before writing them out.

comment:22 Changed 4 weeks ago by asn

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.