go-modules/packages: Run unit tests under subdirs by bdd · Pull Request #173702 · NixOS/nixpkgs

bdd · 2022-05-20T03:50:21Z

Bug Fix:

Due to the way buildGoDir function was repurposed to also run go test, if checkFlags was defined, go test was ran only at the top
level directory.
Only the first element of checkFlags array would get passed to the
go test command as arguments.
Now the first parameter to buildGoDir is handled as the command.
If the command is "test" checkFlags get passed as arguments along
with other build flags like ldflags, tags, etc.

Readability:

Iteratively build a flag array in buildGoDir instead of single long
variable expansion command line.
Bash style: Single line local assignment of positional parameters.

Fix broken packages with newly enabled tests:

kompose: tests require git
gitbatch: test require git & a writable HOME. disable tests requiring
non-localhost networking.
delve: tests require source to be a git repo. fix fortify issues with
cgo.
go-mtpfs: disable tests requiring an android devices connected over
USB.
mmake: disable test. almost all require non-localhost networking.
skeema: tests require coreutils and substitutions. disable tests
requiring non-localhost networking
gucci: disable integrations tests.

Description of changes

Things done

bdd · 2022-05-20T04:01:33Z

Continuation of #171177.

Fixes the test breakages unearthed by the subdirectory fix.

@NixOS/nixos-release-managers: Should build fixes be split into to per-package commits or should they stay as part of this a single commit?

aaronjheng · 2022-05-20T13:46:10Z

Great work.

aaronjheng · 2022-05-20T13:48:29Z

Since checkFlags never works, why not use testFlags instead?

aaronjheng · 2022-05-20T14:02:23Z

I belive most of these checkFlags are added by me. Sorry about that.

The following packages also need to fix:

kube-router
temporal
goconvey
mysqld_exporter
docker-credential-gcr
cloud-sql-proxy

I would like to help to fix it.

bdd · 2022-05-20T16:39:41Z

Since checkFlags never works, why not use testFlags instead?

Well, this change fixes checkFlags.

bdd · 2022-05-20T16:45:48Z

I belive most of these checkFlags are added by me. Sorry about that.

The following packages also need to fix:

kube-router

temporal

goconvey

mysqld_exporter

docker-credential-gcr

cloud-sql-proxy

I would like to help to fix it.

You may opt for dropping checkFlags = [ "-short" ]; from these in the master and re-enable tests for their subdirectories. I'm fairly certain they won't break, otherwise it'd be caught by the previous staging-next Hydra run.

dasJ · 2022-05-21T00:00:12Z

The question about whether multiple commits are required is something I cannot really answer because it's not related to the release process.
While I think it's mergable as-is, multiple commits would be cleaner if you want to go the extra steps

bdd · 2022-05-31T21:54:37Z

@ofborg build kompose

pkgs/development/tools/delve/default.nix

Bug: Due to the way `buildGoDir` function was repurposed to also run `go test`, if `checkFlags` was defined, `go test` was ran only at the top level directory. Only the first element of `checkFlags` array would get passed to the `go test` command as arguments. Fix: Now the first parameter to `buildGoDir` is handled as the command. If the command is "test" `checkFlags` get passed as arguments along with other build flags like ldflags, tags, etc. Readability: - Iteratively build a flag array in `buildGoDir` instead of single long variable expansion command line. - Bash style: Single line local assignment of positional parameters.

The version of Ginkgo it relies on might be the problem. Bug fixed by NixOS#173702 runs the previously skipped tests for this package.

- Disable the tests requiring access to gitlab.com. - Add coreutils to `nativeBuildInputs` for printf and echo binaries. - Fix hard coded paths to coreutils binaries. Bug fixed by NixOS#173702 runs the previously skipped tests for this package.

Bug fixed by NixOS#173702 runs the previously skipped tests for this package.

These test are written in a way that they don't skip themselves if they cannot find an Android device attached over USB to the running host. Bug fixed by NixOS#173702 runs the previously skipped tests for this package.

Disable the tests requiring access to gitlab.com Bug fixed by NixOS#173702 runs the previously skipped tests for this package.

Bug fixed by NixOS#173702 runs the previously skipped tests for this package.

bdd · 2022-06-04T19:58:06Z

Note to committer: Do not squash.

bdd · 2022-06-10T23:32:07Z

@zowoq may I get your review? In the previous iteration of this, you provided valuable comments. I think this PR addresses unearthing of packages with broken test that was previously skipped (unintentionally). We're out of ZHF, so maybe we can try this in staging?

Mic92 · 2022-06-26T06:06:46Z

Looks like an eval error?

% nix-review pr 173702
$ git -c fetch.prune=false fetch --no-tags --force https://github.com/NixOS/nixpkgs staging:refs/nixpkgs-review/0 pull/173702/head:refs/nixpkgs-review/1
remote: Enumerating objects: 37310, done.
remote: Counting objects: 100% (11221/11221), done.
remote: Compressing objects: 100% (37/37), done.
remote: Total 37310 (delta 11196), reused 11188 (delta 11184), pack-reused 26089
Receiving objects: 100% (37310/37310), 59.45 MiB | 24.67 MiB/s, done.
Resolving deltas: 100% (25850/25850), completed with 2706 local objects.
From https://github.com/NixOS/nixpkgs
   7f5523d1333..6905fbbcef2  staging               -> refs/nixpkgs-review/0
 + 90e3c08600e...9d09650cef6 refs/pull/173702/head -> refs/nixpkgs-review/1  (forced update)
$ git worktree add /home/joerg/.cache/nixpkgs-review/pr-173702/nixpkgs 6905fbbcef2f0b123d55a73de83bc4d594c41834
Preparing worktree (detached HEAD 6905fbbcef2)
Updating files: 100% (30856/30856), done.
HEAD is now at 6905fbbcef2 Merge staging-next into staging
$ git merge --no-commit --no-ff 9d09650cef6cf183500d3e4c8ae0356fa0e878e4
Auto-merging pkgs/development/go-modules/generic/default.nix
Auto-merging pkgs/development/tools/delve/default.nix
Auto-merging pkgs/tools/system/skeema/default.nix
Automatic merge went well; stopped before committing as requested
error: Function called without required argument "testVersion" at /home/joerg/.cache/nixpkgs-review/pr-173702/nixpkgs/pkgs/development/tools/jira-cli-go/default.nix:1
(use '--show-trace' to show detailed location information)
nix --experimental-features nix-command --system x86_64-linux eval --json --impure --expr (import /nix/store/p8yz74kbcp1nprrcmxkd2qaczr7givwn-nixpkgs-review-2.7.0/lib/python3.9/site-packages/nixpkgs_review/nix/evalAttrs.nix { allowAliases = false; attr-json = /tmp/tmpdj2jfqm8; }) failed to run, /tmp/tmpdj2jfqm8 was stored inspection
https://github.com/NixOS/nixpkgs/pull/173702 failed to build
$ git worktree prune
nix-review pr 173702  15,41s user 7,21s system 42% cpu 53,838 total

@ofborg eval

bdd · 2022-06-26T15:02:45Z

Looks like an eval error?

[...]
error: Function called without required argument "testVersion" at /home/joerg/.cache/nixpkgs-review/pr-173702/nixpkgs/pkgs/development/tools/jira-cli-go/default.nix:1
[...]

I'm confused. This PR neither touches the failing package nor anything that'd affect the evaluation of testVersion which is an alias for testers.testVersion as defined in toplevel/aliases.nix.

My caveman test doesn't catch an eval failure:

% git reset --hard upstream/staging
HEAD is now at fe5d77ff39b liburing: 2.1 -> 2.2
% curl -LSsf https://patch-diff.githubusercontent.com/raw/NixOS/nixpkgs/pull/173702.patch | git am
Applying: go-modules/packages: Run unit tests under subdirs
Applying: gucci: Disable integration tests.
Applying: skeema: Disable tests requiring network & fix deps
Applying: mmake: Disable non-localhost network access tests
Applying: delve: Fix tests on Linux, disable tests on Darwin
Applying: go-mtpfs: Disable tests req'ing USB attached devs
Applying: gitbatch: Fix tests requiring .git & writable HOME
Applying: kompose: Fix test dependencies
% nix eval .#jira-cli-go.name
"jira-cli-go-0.3.0"

bdd · 2022-06-26T17:37:05Z

@ofborg eval
@ofborg build

bdd · 2022-06-26T18:05:29Z

@Mic92 thanks to @vcunat I think the problem nixpkgs-review hits this eval issue is because it's disabling allowAliases option; which is what testVersion is. ...but as I said above, this PR doesn't touch any packages relying on aliases.

BTW, I also filed #179226 to remove use of testVersion from master.

bdd · 2022-06-27T20:26:33Z

@ofborg eval

Mic92 · 2022-06-28T05:49:42Z

I could not test all packages because of broken dependencies, but these go ones here failed (also maybe not because of the change).

@ofborg build reproxy
@ofborg build s5cmd
@ofborg build shopify-themekit
@ofborg build toxiproxy
@ofborg build victoriametrics

Mic92 · 2022-06-28T06:30:39Z

Mhm, weird. They failed to build, but maybe it was some concurrency issue.

bdd requested review from FRidh, Mic92, jonringer, kalbasit and zowoq as code owners May 20, 2022 03:50

github-actions bot added 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: python Python is a high-level, general-purpose programming language. labels May 20, 2022

bdd force-pushed the go-checkFlags branch 2 times, most recently from 0f07c85 to d676e7c Compare May 20, 2022 03:53

github-actions bot removed the 6.topic: python Python is a high-level, general-purpose programming language. label May 20, 2022

bdd requested a review from vcunat May 20, 2022 23:00

aaronjheng mentioned this pull request May 21, 2022

Stop using ineffective check flags #173835

Merged

13 tasks

bdd force-pushed the go-checkFlags branch from 2df433d to 9945ea3 Compare May 31, 2022 05:41

bdd requested a review from SuperSandro2000 May 31, 2022 23:03

SuperSandro2000 reviewed Jun 1, 2022

View reviewed changes

pkgs/development/tools/delve/default.nix Outdated Show resolved Hide resolved

bdd added 8 commits June 1, 2022 00:52

gucci: Disable integration tests.

17672c8

The version of Ginkgo it relies on might be the problem. Bug fixed by NixOS#173702 runs the previously skipped tests for this package.

mmake: Disable non-localhost network access tests

7f2ef31

Bug fixed by NixOS#173702 runs the previously skipped tests for this package.

delve: Fix tests on Linux, disable tests on Darwin

032f261

Bug fixed by NixOS#173702 runs the previously skipped tests for this package.

go-mtpfs: Disable tests req'ing USB attached devs

aee4df7

These test are written in a way that they don't skip themselves if they cannot find an Android device attached over USB to the running host. Bug fixed by NixOS#173702 runs the previously skipped tests for this package.

gitbatch: Fix tests requiring .git & writable HOME

a3205db

Disable the tests requiring access to gitlab.com Bug fixed by NixOS#173702 runs the previously skipped tests for this package.

kompose: Fix test dependencies

9d09650

Bug fixed by NixOS#173702 runs the previously skipped tests for this package.

bdd force-pushed the go-checkFlags branch from 9945ea3 to 9d09650 Compare June 1, 2022 01:08

bdd requested a review from SuperSandro2000 June 1, 2022 01:08

aaronjheng mentioned this pull request Jun 25, 2022

git-town: use buildGoModule #176200

Merged

13 tasks

bdd requested review from a team and Frostman June 25, 2022 16:32

Mic92 mentioned this pull request Jun 27, 2022

Merge master #179293

Merged

13 tasks

Mic92 merged commit 4f8a04f into NixOS:staging Jun 28, 2022

bdd deleted the go-checkFlags branch December 4, 2022 00:20

Uh oh!

Conversation

bdd commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

bdd commented May 20, 2022

Uh oh!

aaronjheng commented May 20, 2022

Uh oh!

aaronjheng commented May 20, 2022

Uh oh!

aaronjheng commented May 20, 2022

Uh oh!

bdd commented May 20, 2022

Uh oh!

bdd commented May 20, 2022

Uh oh!

dasJ commented May 21, 2022

Uh oh!

bdd commented May 31, 2022

Uh oh!

Uh oh!

bdd commented Jun 4, 2022

Uh oh!

bdd commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mic92 commented Jun 26, 2022

Uh oh!

bdd commented Jun 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdd commented Jun 26, 2022 • edited by SuperSandro2000 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdd commented Jun 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdd commented Jun 27, 2022

Uh oh!

Mic92 commented Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mic92 commented Jun 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

bdd commented May 20, 2022 •

edited

Loading

bdd commented Jun 10, 2022 •

edited

Loading

bdd commented Jun 26, 2022 •

edited

Loading

bdd commented Jun 26, 2022 •

edited by SuperSandro2000

Loading

bdd commented Jun 26, 2022 •

edited

Loading

Mic92 commented Jun 28, 2022 •

edited

Loading