Skip to content

Comments

[WIP]: nixosTests.nixos-test-driver#154168

Draft
Synthetica9 wants to merge 15 commits intoNixOS:masterfrom
Synthetica9:test-driver-tests
Draft

[WIP]: nixosTests.nixos-test-driver#154168
Synthetica9 wants to merge 15 commits intoNixOS:masterfrom
Synthetica9:test-driver-tests

Conversation

@Synthetica9
Copy link
Member

@Synthetica9 Synthetica9 commented Jan 9, 2022

Motivation for this change

Add tests for the nixos test driver (very meta).
Coverage is collected in a really hacky way atm, if anyone has tips how to do this nicely I'd be all ears.
As discussed in #146905

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Jan 9, 2022
@Synthetica9
Copy link
Member Author

Synthetica9 commented Jan 9, 2022

Current coverage status:

Name                               Stmts   Miss  Cover
------------------------------------------------------
test_driver/__init__.py               42      6    86%
test_driver/driver.py                 98     12    88%
test_driver/logger.py                 76      4    95%
test_driver/machine.py               519    132    75%
test_driver/polling_condition.py      52      0   100%
test_driver/vlan.py                   33      5    85%
------------------------------------------------------
TOTAL                                820    159    81%

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 9, 2022
@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Jan 11, 2022
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. labels Jan 11, 2022
@Synthetica9 Synthetica9 mentioned this pull request Jan 11, 2022
Draft
15 tasks
@Synthetica9 Synthetica9 mentioned this pull request Jan 20, 2022
Open
13 tasks
@ofborg ofborg bot added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Jan 20, 2022
@ajs124 ajs124 requested a review from marijanp January 20, 2022 22:28
marijanp
marijanp reviewed Jan 25, 2022
View reviewed changes
Copy link
Contributor

@marijanp marijanp left a comment

Choose a reason for hiding this comment

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

Ok I reviewed parts of this PR now and it seems to me that you made a lot of changes which are not directly related to adding test-driver tests or collecting coverage data. (Correct me if I'm wrong)
A lot of these changes/ refactorings are desireable, and they might fix some bugs/ issues you uncovered with the new tests. These changes should definetly be in this PR.
But we should get rid of all other changes/refactorings and put them in a subsequent PR. So we can focus on the essence of introducing tests for the test-driver, making the review process a lot easier and I'm sure we will be able to get this merged pretty fast.

nixos/lib/test-driver/test_driver/logger.py Outdated
Copy link
Contributor

@marijanp marijanp Jan 25, 2022

Choose a reason for hiding this comment

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

Where did close move?

Copy link
Member Author

@Synthetica9 Synthetica9 Jan 25, 2022

Choose a reason for hiding this comment

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

It wasn't used anywhere internally nor in any test. Maybe a better solution would be to convert logger into a context manager and use that properly

nixos/lib/test-driver/test_driver/logger.py Outdated
Comment on lines +13 to +26
Copy link
Contributor

@marijanp marijanp Jan 25, 2022

Choose a reason for hiding this comment

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

What is the reason for factoring these functions out?

Copy link
Member Author

@Synthetica9 Synthetica9 Jan 25, 2022

Choose a reason for hiding this comment

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

They were only used internally, didn't use anything in the Logger instance, and didn't have close enough relation to the Logger class (imo) to justify being @staticmethod

nixos/lib/test-driver/test_driver/machine.py Outdated
Copy link
Contributor

@marijanp marijanp Jan 25, 2022

Choose a reason for hiding this comment

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

Ok, i see why you removed the previous argument

Copy link
Member Author

@Synthetica9 Synthetica9 Jan 25, 2022

Choose a reason for hiding this comment

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

(it was broken on top of being legacy, see:

In [1]: (
   ...:     1
   ...:     if False else
   ...:     "aaa"
   ...:     "bbb"
   ...:     "ccc"
   ...: )
Out[1]: 'aaabbbccc'

nixos/lib/test-driver/test_driver/machine.py Outdated
Copy link
Contributor

@marijanp marijanp Jan 25, 2022

Choose a reason for hiding this comment

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

why refactor format now?

@Synthetica9
nixos/test-driver: refactor CHAR_TO_KEY
a6c52d9
@Synthetica9
nixos/test-driver: remove unused and incorrect no_reboot option
fe93b79
@Synthetica9
nixos/test-driver: rewrite several functions
bf70161 
Mostly in terms of smaller building blocks like `retry` or `wait_until_succeeds`.
Also adds `timeout` to all functions that use `retry`.
@Synthetica9
nixos/test-driver: small cleanups
d74d1d2
@Synthetica9
nixos/test-driver: don't hang when vm exits unexpectedly
72faad0
@Synthetica9
[WIP]: nixosTests.nixos-test-driver
9440855
@Synthetica9
nixos/test-driver: use f-string formatting where possible
0f84f88
@Synthetica9
nixos/test-driver: use explicit quoting instead of make_command
819ddaa 
avoid a few 'shell injection' style bugs while we're at it
@Synthetica9
nixosTests.nixos-test-driver: [WIP]
aab7842
@Synthetica9
nixos/test-driver: check expression parses before executing
03ab2f8
@Synthetica9
nixos/test-driver: simplifiy Machine.execute
be1c470
@Synthetica9
nixos/test-driver: remove methods from Logger that are
e60a78c 
- Unused
- Don't need to be a class/static method
@Synthetica9
nixos/test-driver: remove wrongly committed .mutmut-cache
3d446d0
@Synthetica9
nixos/test-driver: add and export Driver.must_raise
9b481db
@Synthetica9
[WIP]
ac042d6
@github-actions github-actions bot removed the 6.topic: python Python is a high-level, general-purpose programming language. label Jan 26, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. and removed 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. labels Jan 26, 2022
@Synthetica9 Synthetica9 mentioned this pull request Jan 28, 2022
Open
13 tasks
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 31, 2022
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@marijanp marijanp marijanp left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Synthetica9 @marijanp @wegank