Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RFC] Use meta.tests to link from packages to the tests that test them #44439

Merged
merged 1 commit into from
Nov 6, 2018
Merged

[RFC] Use meta.tests to link from packages to the tests that test them #44439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions doc/meta.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,61 @@ meta.platforms = stdenv.lib.platforms.linux;
</para>
</listitem>
</varlistentry>
<varlistentry>
<term>
<varname>tests</varname>
</term>
<listitem>
<para>
An attribute set with as values tests. A test is a derivation, which
builds successfully when the test passes, and fails to build otherwise. A
derivation that is a test requires some <literal>meta</literal> elements
to be defined: <literal>needsVMSupport</literal> (automatically filled-in
Copy link
Member

Choose a reason for hiding this comment

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

This should probably not be created, since this is already covered by the requiredFeatures of the derivation.

for NixOS tests) and <literal>timeout</literal>.
</para>
<para>
The NixOS tests are available as <literal>nixosTests</literal> in
parameters of derivations. For instance, the OpenSMTPD derivation
includes lines similar to:
<programlisting>
{ /* ... */, nixosTests }:
{
# ...
meta.tests = {
basic-functionality-and-dovecot-integration = nixosTests.opensmtpd;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this has been discussed before (can't find it with a quick skim), but is there a reason to make this a set instead of a simple list? I feel like having to give every test a new name only reduces redundancy. If there is additional information (like what exactly is tested in this case), maybe renaming the original test or a comment would be better?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had originally implemented this as a list, and @Profpatsch's comment (#44439 (comment)) made me notice it could be written as an attrset. At the time, it was a good thing, because tests needed to be import-ed, and the additional parenthesis in lists are often a source of errors.

Now that the tests are easily accessible from the fixpoint… I don't have any strong sentiment about it. @Profpatsch maybe has?

Copy link
Member

@Profpatsch Profpatsch Oct 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

A list is kinda hard to work with, it can only be mapped over and replaced by an other list. There is no (sane) way to reference single elements in it. So if there should ever be the need to work with one of these tests especially (e.g. for overriding or removing it), you don’t have a good reference to work with (and no, the test’s name attribute is not a good reference).

Same problem with environment.systemPackages for example.

Copy link
Member

Choose a reason for hiding this comment

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

I still don't like the "UX" of attribute sets here very much, but your argument makes sense. Using inherit would at least remove the redundancy.

So as a summary I'm not quite happy but have no better idea.

};
}
</programlisting>
</para>
</listitem>
</varlistentry>
<varlistentry>
<term>
<varname>timeout</varname>
</term>
<listitem>
<para>
A timeout (in seconds) for building the derivation. If the derivation
takes longer than this time to build, it can fail due to breaking the
timeout. However, all computers do not have the same computing power,
hence some builders may decide to apply a multiplicative factor to this
value. When filling this value in, try to keep it approximately
consistent with other values already present in
<literal>nixpkgs</literal>.
</para>
</listitem>
</varlistentry>
<varlistentry>
<term>
<varname>needsVMSupport</varname>
</term>
<listitem>
<para>
A boolan that states whether the derivation requires build-time support
for Virtual Machine to build successfully.
</para>
</listitem>
</varlistentry>
<varlistentry>
<term>
<varname>hydraPlatforms</varname>
Expand Down
4 changes: 3 additions & 1 deletion nixos/lib/testing.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ in rec {
mkdir -p $out/coverage-data
mv $i $out/coverage-data/$(dirname $(dirname $i))
done
''; # */
'';

meta.needsVMSupport = true;
};


Expand Down
2 changes: 2 additions & 0 deletions nixos/tests/opensmtpd.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,4 +120,6 @@ import ./make-test.nix {
$smtp2->waitUntilFails('smtpctl show queue | egrep .');
$client->succeed('check-mail-landed >&2');
'';

meta.timeout = 30;
}
4 changes: 4 additions & 0 deletions pkgs/servers/mail/dovecot/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{ stdenv, lib, fetchurl, perl, pkgconfig, systemd, openssl
, bzip2, zlib, lz4, inotify-tools, pam, libcap
, clucene_core_2, icu, openldap, libsodium, libstemmer, cyrus_sasl
, nixosTests
# Auth modules
, withMySQL ? false, mysql
, withPgSQL ? false, postgresql
Expand Down Expand Up @@ -74,5 +75,8 @@ stdenv.mkDerivation rec {
description = "Open source IMAP and POP3 email server written with security primarily in mind";
maintainers = with stdenv.lib.maintainers; [ peti rickynils fpletz ];
platforms = stdenv.lib.platforms.unix;
tests = {
opensmtpd-interaction = nixosTests.opensmtpd;
};
};
}
5 changes: 4 additions & 1 deletion pkgs/servers/mail/opensmtpd/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{ stdenv, lib, fetchurl, fetchpatch, autoconf, automake, libtool, bison
, libasr, libevent, zlib, libressl, db, pam
, libasr, libevent, zlib, libressl, db, pam, nixosTests
}:

stdenv.mkDerivation rec {
Expand Down Expand Up @@ -61,5 +61,8 @@ stdenv.mkDerivation rec {
license = licenses.isc;
platforms = platforms.linux;
maintainers = with maintainers; [ rickynils obadz ekleog ];
tests = {
basic-functionality-and-dovecot-interaction = nixosTests.opensmtpd;
};
};
}
12 changes: 10 additions & 2 deletions pkgs/stdenv/generic/check-meta.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,16 @@ let
platforms = listOf (either str lib.systems.parsedPlatform.types.system);
hydraPlatforms = listOf str;
broken = bool;
# TODO: refactor once something like Profpatsch's types-simple will land
tests = attrsOf (mkOptionType {
name = "test";
check = x: isDerivation x &&
x ? meta.timeout &&
x ? meta.needsVMSupport;
merge = lib.options.mergeOneOption;
});
needsVMSupport = bool;
timeout = int;

# Weirder stuff that doesn't appear in the documentation?
knownVulnerabilities = listOf str;
Expand All @@ -184,8 +194,6 @@ let
isIbusEngine = bool;
isGutenprint = bool;
badPlatforms = platforms;
# Hydra build timeout
timeout = int;
};

checkMetaAttr = k: v:
Expand Down
20 changes: 20 additions & 0 deletions pkgs/top-level/all-packages.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,26 @@ with pkgs;

common-updater-scripts = callPackage ../common-updater/scripts.nix { };

### Push NixOS tests inside the fixed point

nixosTests =
let
# TODO(Ericson2314,ekleog): Check this will work correctly with cross-
system = builtins.currentSystem;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn’t work, I think. cc @Ericson2314

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really know: NixOS test derivations are supposed to be built on the local system, which I'd guess to be builtins.currentSystem? Not sure at all though, so cc @Ericson2314 indeed :) I.must find it hard to imagine what's the cross-derivation story of NIxOS tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Ericson2314 ping :) (2 months sounds like a reasonable waiting time :p)

rawTests = (import ../../nixos/release.nix {
nixpkgs = pkgs;
}).tests;
testNames = builtins.attrNames rawTests;
filteredList = builtins.filter
(test: rawTests.${test} ? ${system})
testNames;
finalList = map
(test: { name = test; value = rawTests.${test}.${system}; })
filteredList;
finalTests = builtins.listToAttrs finalList;
in
finalTests;

### BUILD SUPPORT

autoreconfHook = makeSetupHook
Expand Down