Skip to content

Port gnome3 like tests to python and udisks2#72860

Merged
worldofpeace merged 5 commits intoNixOS:masterfrom
worldofpeace:port-gnome3y-tests
Nov 7, 2019
Merged

Port gnome3 like tests to python and udisks2#72860
worldofpeace merged 5 commits intoNixOS:masterfrom
worldofpeace:port-gnome3y-tests

Conversation

@worldofpeace
Copy link
Copy Markdown
Contributor

@worldofpeace worldofpeace commented Nov 5, 2019 •
edited
Loading

Motivation for this change

#72828

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Nov 5, 2019
jtojnar
jtojnar reviewed Nov 5, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

It would be nice to have a function for gnome-desktop-testing-runner too.

nixos/tests/packagekit.nix Outdated
Copy link
Copy Markdown
Member

@jtojnar jtojnar Nov 5, 2019

Choose a reason for hiding this comment

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

Was the dbus API added or will it come later?

Copy link
Copy Markdown
Contributor Author

@worldofpeace worldofpeace Nov 5, 2019

Choose a reason for hiding this comment

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

Do you mean in the testing driver? I don't think so, but I have some great ideas of all the things that should be added there. Currently they've tried to go for a 1:1 port from the perl one.

@worldofpeace
Copy link
Copy Markdown
Contributor Author

worldofpeace commented Nov 5, 2019 •
edited
Loading

It would be nice to have a function for gnome-desktop-testing-runner too.

I can make it similar to #34987 (comment) but maybe makeGnomeInstalledTest and have them in one flat file like what I did for flashback

(description is Make NixOS test for gnome-desktop-testing-runner using software)

@worldofpeace
Copy link
Copy Markdown
Contributor Author

worldofpeace commented Nov 6, 2019

@jtojnar I've added a gnome-installed-tests.nix with the function and the tests.
That does make all the tests under nixosTests.gnome-installed-tests.*.

Would you prefer having everything in one file like I've done, or perhaps the colord = makeGnomeInstalledTests could be split out?

@jtojnar
Copy link
Copy Markdown
Member

jtojnar commented Nov 6, 2019

I think one file per test is still a good idea, some of them are slightly more complicated, recall also python-packages.nix. The really trivial ones could perhaps be created directly in all-tests.nix.

I am also not sure about the GnomeInstalledTests name. While it originated in GNOME project, it spread to various freedesktop projects as well.

@worldofpeace
Copy link
Copy Markdown
Contributor Author

worldofpeace commented Nov 6, 2019

I think one file per test is still a good idea, some of them are slightly more complicated, recall also python-packages.nix. The really trivial ones could perhaps be created directly in all-tests.nix.

I am also not sure about the GnomeInstalledTests name. While it originated in GNOME project, it spread to various freedesktop projects as well.

I just wanted the function name to express that it's just for projects that use gnome-desktop-testing.

I will change the structure though, python-packages.nix was kinda a mess and all-tests.nix is structured that way for a similar reason.

@FRidh
Copy link
Copy Markdown
Member

FRidh commented Nov 6, 2019

I am a bit confused by the moving of the tests. In the package set, I thought the plan was to get rid of the gnome3 namespace. Why not with tests then?

@jtojnar
Copy link
Copy Markdown
Member

jtojnar commented Nov 6, 2019

gnome-desktop-testing is just a single implementation of the test runner but the installed spec itself is not specific to GNOME. There is at least ibus-desktop-testing-runner. I plan to add that in #71442

@worldofpeace
Copy link
Copy Markdown
Contributor Author

worldofpeace commented Nov 6, 2019

From IRC:

@worldofpeace
jtojnar: what are the requested changes in #72860 (comment)? I just wanted a good overview of the installed tests specifically, and having them under an attr nixosTests.gnome-installed-tests.* just made that easy. No relation to the gnome3 attr deprecation

@jtojnar
worldofpeace I would just use nixosTests.installed-tests, and maybe pester upstream to drop gnome from its name when I have some time

@worldofpeace
jtojnar: did that 👍️

jtojnar
jtojnar reviewed Nov 6, 2019
View reviewed changes
jtojnar
jtojnar reviewed Nov 6, 2019
View reviewed changes
nixos/tests/udisks2.nix Outdated
Copy link
Copy Markdown
Member

@jtojnar jtojnar Nov 6, 2019

Choose a reason for hiding this comment

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

Why space this around, "${stick}" is much shorter than the line below.

Copy link
Copy Markdown
Contributor Author

@worldofpeace worldofpeace Nov 6, 2019

Choose a reason for hiding this comment

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

black fails on line length, as this becomes a nix store path so it demands I format it this way. We should probably make that more relaxed. Similarly I've had to format makeInstalledTest testscript injection specifically with concatenating multi-line strings so it doesn't error out on it not being pretty.

jtojnar
jtojnar reviewed Nov 6, 2019
View reviewed changes
nixos/tests/installed-tests/gnome-photos.nix Outdated
Copy link
Copy Markdown
Member

@jtojnar jtojnar Nov 6, 2019

Choose a reason for hiding this comment

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

In the future, we should probably have a module for this. Depends on #54150

nixos/tests/installed-tests/default.nix Outdated
Copy link
Copy Markdown
Member

@jtojnar jtojnar Nov 6, 2019

Choose a reason for hiding this comment

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

Maybe we should make this an array in line with structuredAttrs proposal. I can imagine python test driver to accept list of args like Python’s subprocess module does.

@jtojnar jtojnar mentioned this pull request Nov 6, 2019
Merged
jtojnar
jtojnar reviewed Nov 6, 2019
View reviewed changes
@worldofpeace
Copy link
Copy Markdown
Contributor Author

worldofpeace commented Nov 6, 2019

@GrahamcOfBorg test udisks2 fontconfig-default-fonts packagekit

@worldofpeace worldofpeace merged commit a46c781 into NixOS:master Nov 7, 2019
@worldofpeace worldofpeace deleted the port-gnome3y-tests branch November 7, 2019 00:44
@worldofpeace worldofpeace mentioned this pull request Nov 7, 2019
Merged
10 tasks
@tfc tfc mentioned this pull request Nov 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jtojnar jtojnar jtojnar left review comments

@flokli flokli Awaiting requested review from flokli

Assignees

No one assigned

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@worldofpeace @jtojnar @FRidh