On Thu, May 16, 2024 at 04:54:58PM +0300, Ivanov Mikhail wrote:
>
>
> 5/8/2024 1:38 PM, Mickaël Salaün wrote:
> > On Thu, Apr 11, 2024 at 06:58:34PM +0300, Ivanov Mikhail wrote:
> > >
> > > 4/8/2024 4:08 PM, Günther Noack wrote:
> > > > > + {
> > > > > + TH_LOG("Failed to create socket: %s", strerror(errno));
> > > > > + }
> > > > > + EXPECT_EQ(0, close(fd));
> > > > > + }
> > > > > +}
> > > >
> > > > This is slightly too much logic in a test helper, for my taste,
> > > > and the meaning of the true/false argument in the main test below
> > > > is not very obvious.
> > > >
> > > > Extending the idea from above, if test_socket() was simpler, would it
> > > > be possible to turn these fixtures into something shorter like this:
> > > >
> > > > ASSERT_EQ(EAFNOSUPPORT, test_socket(AF_UNSPEC, SOCK_STREAM, 0));
> > > > ASSERT_EQ(EACCES, test_socket(AF_UNIX, SOCK_STREAM, 0));
> > > > ASSERT_EQ(EACCES, test_socket(AF_UNIX, SOCK_DGRAM, 0));
> > > > ASSERT_EQ(EACCES, test_socket(AF_INET, SOCK_STREAM, 0));
> > > > // etc.
> >
> > I'd prefer that too.
> >
> > > >
> > > > Would that make the tests easier to write, to list out the table of
> > > > expected values aspect like that, rather than in a fixture?
> > > >
> > > >
> > >
> > > Initially, I conceived this function as an entity that allows to
> > > separate the logic associated with the tested methods or usecases from
> > > the logic of configuring the state of the Landlock ruleset in the
> > > sandbox.
> > >
> > > But at the moment, `test_socket_create()` is obviously a wrapper over
> > > socket(2). So for now it's worth removing unnecessary logic.
> > >
> > > But i don't think it's worth removing the fixtures here:
> > >
> > > * in my opinion, the design of the fixtures is quite convenient.
> > > It allows you to separate the definition of the object under test
> > > from the test case. E.g. test protocol.create checks the ability of
> > > Landlock to restrict the creation of a socket of a certain type,
> > > rather than the ability to restrict creation of UNIX, TCP, UDP...
> > > sockets
> >
> > I'm not sure to understand, but we need to have positive and negative
> > tests, potentially in separate TEST_F().
>
> I mean we can use fixtures in order to not add ASSERT_EQ for
> each protocol, as suggested by Günther. It's gonna look like this:
>
> ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
> ASSERT_EQ(EACCES, test_socket(&self->srv0));
>
> I think it would make the test easier to read, don't you think so?
Yes, this looks good.
