Opened 4 years ago

Last modified 2 years ago

#17081 needs_revision enhancement

Improve coverage on src/common/sandbox.c

Reported by: rjunior Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-tests, testing, SponsorS-deferred
Cc: Actual Points:
Parent ID: Points: medium
Reviewer: Sponsor:

Description

The changes are in the branch "sandbox_tests"

​​​​https://github.com/twstrike/tor_for_patching/tree/sandbox_tests

Child Tickets

Change History (18)

comment:1 Changed 4 years ago by rjunior

Status: newneeds_review

comment:2 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-final

comment:3 Changed 4 years ago by nickm

Keywords: 028-triaged added

comment:4 Changed 4 years ago by nickm

Sponsor: SponsorS

comment:5 Changed 4 years ago by nickm

Points: small

mark these testing tickets in needs_review as 'small work remaining'

comment:6 Changed 4 years ago by nickm

Points: smallmedium
Status: needs_reviewneeds_revision

I've made some comments on the github repository.

The biggest issue here is that the tests don't test whether the sandboxing code actually works or not: they only test that it returns a "success" status code.

comment:7 Changed 4 years ago by olabini

Yes, these tests actually only test the "stubbed" sandbox methods. I couldn't figure out a way of testing the actual sandbox code without breaking a whole bunch of stuff. (As you might see, all the current tests are actually inside of an #else whose true-axis is #ifdef USE_LIBSECCOMP

As such, it might not be valuable to merge these tests.

In the meantime I have fixed the space issues and warning issues in that branch.

comment:8 Changed 4 years ago by olabini

Severity: Normal

Same question on protocol - should I change the status back to needs_review?

comment:9 Changed 4 years ago by nickm

Status: needs_revisionneeds_review

Yup. When you think it's ready for another round of review, it goes back in needs-review

comment:10 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final
Status: needs_reviewneeds_revision

As such, it might not be valuable to merge these tests.

Yeah, the stubbed versions of these aren't so helpful to test. Deferring more here for 029.

comment:11 Changed 4 years ago by isabela

Sponsor: SponsorSSponsorS-can

comment:12 Changed 4 years ago by isabela

Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

tickets market to be removed from milestone 029

comment:13 Changed 4 years ago by nickm

Keywords: SponsorS-deferred added
Sponsor: SponsorS-can

Remove the SponsorS status from these items, which we already decided to defer from 0.2.9. add the SponsorS-deferred tag instead in case we ever want to remember which ones these were.

comment:14 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:15 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:16 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:17 Changed 3 years ago by nickm

Keywords: 028-triaged removed

comment:18 Changed 2 years ago by nickm

Keywords: tor-tests added
Note: See TracTickets for help on using tickets.