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.

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

Already on GitHub? Sign in to your account

nixos/tests/keymap: improve keymap tests #39400

Merged
merged 1 commit into from Apr 24, 2018
Merged

Conversation

xeji
Copy link
Contributor

@xeji xeji commented Apr 24, 2018

Motivation for this change

Fix #39330. Simplify tests, prevent timeouts and non-deterministic hydra failures.
Hopefully this will stabilize the tests on hydra - let's try and see.

Please backport to 18.03 because failing keymap tests have delayed the channels there as well.

Things done
  • Simplify and redesign test logic: a single reader script now waits for all test characters to be typed (previously, a new reader script was started on the guest for each character)
  • Improve host-guest synchronization to reduce waiting and timeout risks
  • Xorg test: wait for xterm window to open (waitForWindow())
  • Xorg test: use xdotool to make sure xterm has focus (from nixos/tests/keymap: Workaround xterm not in focus聽#33719, thanks @srhb)
  • Add some logging to help future debugging
  • Verify that all 6 keymap tests pass locally, even when run in parallel with high CPU load (since I don't own a hydra simulator 馃榾)
Things that could be done later
  • Flatten test data sets - I chose to keep test data unchanged for now and have nix flatten it
  • Add a standard synchronization mechanism to the test framework; having to sync by creating and waiting for marker files is a pain and error-prone, and everyone rolls their own...

simplify tests, prevent timeouts and non-deterministic failures
@srhb
Copy link
Contributor

srhb commented Apr 24, 2018

@GrahamcOfBorg test keymap.colemak

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: tests.keymap.colemak

Partial log (click to expand)

syncing
machine: running command: sync
machine: exit status 0
2 out of 2 tests succeeded
test script finished in 18.36s
cleaning up
killing machine (pid 593)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/a3j8c8clkqdvklalzjzmk9d5bkxw2dhi-vm-test-run-keymap-colemak

@adisbladis
Copy link
Member

adisbladis commented Apr 24, 2018

Works great here even under quite extreme load (load average 18) on my laptop using a https://ark.intel.com/products/95451.

Great work! 馃槂

@srhb
Copy link
Contributor

srhb commented Apr 24, 2018

Looks excellent! Thanks!

@srhb srhb merged commit 65abd2e into NixOS:master Apr 24, 2018
srhb pushed a commit that referenced this pull request Apr 24, 2018
simplify tests, prevent timeouts and non-deterministic failures

(cherry picked from commit 84a6e18)
Backport #39400
@xeji xeji deleted the improve-keymap-tests branch April 24, 2018 09:55
@GrahamcOfBorg
Copy link

Failure on aarch64-linux (full log)

Attempted: tests.keymap.colemak

Partial log (click to expand)

cannot build derivation '/nix/store/6fs5pcs38yk83zshbwpd19h067w4llrx-etc.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/4r9i25dqkzfyjflqy9xh7p569jnvi61f-stage-1-init.sh.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/z1hpnas8mh2jhz6z8qy7715hpv971l6i-initrd.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/7d870isrnxysjiryzdfx3z63xh7b7xp0-nixos-system-machine-18.09.git.dd4d1a4.drv': 5 dependencies couldn't be built
cannot build derivation '/nix/store/srdj9y73bdxf0m12gcirwa0jjsa9vd6f-closure-info.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/kqam08i23b5cjblaynj6apxvg344dhiq-run-nixos-vm.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/2nsagj1fqjkn0nb2cp7ywcdav1ymsb0r-nixos-vm.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/cl4kasd95x05d38rzl2vrqsk7ap2mp52-nixos-test-driver-keymap-colemak.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/m26z62v6hi9qvvmq25sqy7q29fg7rsnd-vm-test-run-keymap-colemak.drv': 1 dependencies couldn't be built
锟絒31;1merror:锟絒0m build of '/nix/store/m26z62v6hi9qvvmq25sqy7q29fg7rsnd-vm-test-run-keymap-colemak.drv' failed

@srhb
Copy link
Contributor

srhb commented Apr 24, 2018

People of the future, this is the first Hydra evaluation of nixos:release-18.03 with this commit:

https://hydra.nixos.org/eval/1451135

@srhb
Copy link
Contributor

srhb commented Apr 24, 2018

https://hydra.nixos.org/build/73238407/nixlog/13

Unfortunately, no dice. Shall I revert?

@xeji
Copy link
Contributor Author

xeji commented Apr 24, 2018

Oops. Please revert on 18.03 while I check it out. Looks like an issue with xdotool and the window manager, will probably have to modify a little for 18.03.

srhb pushed a commit that referenced this pull request Apr 24, 2018
@xeji
Copy link
Contributor Author

xeji commented Apr 24, 2018

Thanks @srhb for catching this.

Strangely, the same test succeeded in the same eval for i686: https://hydra.nixos.org/build/73238415
Plus I cannot reproduce the above failure (which definitely looks like a deterministic error) on the same git revision.
The test different derivation hashes on the same commit differ between hydra and my local machine (this seems to be the case for other tests as well, whereas normal packages have identical hashes).
Do we have an impurity in the test framework? 馃槙
Appreciate any ideas how to analyze this...

@srhb
Copy link
Contributor

srhb commented Apr 24, 2018

The tests are quite often impure. And uh, did someone rerun the tests or did I completely misread that there was an error? My old look shows it's fine now. Am I dreaming?

@srhb
Copy link
Contributor

srhb commented Apr 24, 2018

And I'm not sure it looked to me like a deterministic error. Could it be that the property in question does not exist if we ask too fast?

@xeji
Copy link
Contributor Author

xeji commented Apr 24, 2018

You're not dreaming, I saw the error too ... But I doubt someone restarted that job. No idea what happened.

And I'm not sure it looked to me like a deterministic error. Could it be that the property in question does not exist if we ask too fast?

Maybe. The only thing I found in xdotool doc is that windowactivate may not work with all window managers.

@srhb
Copy link
Contributor

srhb commented Apr 24, 2018

@xeji OK, good! And yes, I saw that too, and I think what's going on is that it doesn't work yet during whatever startup is happening. I can't imagine that the VM would be different between our systems and Hydras on that point.

@xeji
Copy link
Contributor Author

xeji commented Apr 24, 2018

Since I do waitForWindow before xdotool, the window is already there. But maybe the WM hasn't decorated and set all of the window's properties yet. I could wrap the xdotool call in a waitUntilSucceeds() loop. Or use windowfocus alternative to windowactivate.

@srhb
Copy link
Contributor

srhb commented Apr 24, 2018

If windowfocus works regardless, that seems like the way to go!

@srhb
Copy link
Contributor

srhb commented Apr 24, 2018

(But if we can find a step to actually waitUntilSucceeds that makes this work, I think that's more than acceptable!)

@xeji
Copy link
Contributor Author

xeji commented Apr 24, 2018

Well the manpage says about windowfocus:

Uses XSetInputFocus which may be ignored by some window managers or programs.

@srhb
Copy link
Contributor

srhb commented Apr 24, 2018

That should be testable in the by spawning another window with focus and switching, right?

@xeji
Copy link
Contributor Author

xeji commented Apr 24, 2018

Good idea, I'll do that.

I haven't seen any case where the old test failed because of losing focus after the second xterm window was removed. Now there's only one fullscreen window. So I wonder if the xdotool is just an extra failsafe that creates more trouble than it prevents...

@srhb
Copy link
Contributor

srhb commented Apr 24, 2018

Oh, good point. I forgot about that. In that case, feel free to nuke it! :-) I thought it would help us find a safe spot to start the test, but that may indeed not be the case at all.

@xeji
Copy link
Contributor Author

xeji commented Apr 24, 2018

Let's wait and see how the tests work run on master. I'll do some more experiments and then make an updated version.

@xeji
Copy link
Contributor Author

xeji commented Apr 24, 2018

Thanks again for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants