-
Notifications
You must be signed in to change notification settings - Fork 4
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set up valtools subtree and move to GitHub Actions #419
Conversation
The test runners will enable coverage when GOCOVERDIR is set.
This file provides required top-level .scorecard.json fields. mpn.scorecard will stitch this and the other json files into a complete object.
Under 'go test -json', details about the test failures are in the action=output records for a test. Relay that output to hopefully give the test runner a better idea of why a test is failing.
The scripts for VT_TEST_RUNNERS need to implement fairly complex handling. When GOCOVERDIR is set, they need to instrument a binary for integration tests and to pass '-args -test.gocoverdir=$d' for unit tests. They need to do custom package selection, separating unit tests from integration tests and removing this valtools subtree. And for unit tests, they need to pass -coverpkgs in order for the tests of one package to count toward the coverage of another package. On top of that, the files of packages without tests are not included in the coverage profiles for unit tests invoked with -test.gocoverdir. Take the following module: |-- bar | `-- bar.go |-- foo | |-- foo.go | `-- foo_test.go `-- go.mod If 'go test -coverprofile coverage.out ./...' is called from the top level, the profile will include entries for bar.go. However, if 'go test -cover ./... -args -test.gocoverdir=$d' followed by 'go tool covdata textfmt -i $d -o coverage.out' is called to create the text format profile, entries for bar.go are _not_ present [*]. Given the above complexity, it's helpful to have a way to verify that the Go files in the final coverage JSON match what's expected. [*] tested under Go 1.22.5, which includes d1cb5c0605 (cmd/go: improve handling of no-test packages for coverage, 2023-05-09)
Two of the tests lead to a checkIsTempDir() call that skips unless on macOS. This is just a small detail of the test (and arguably not even worth checking), so it doesn't make sense to mark the entire test as skipped on other systems.
This is already covered by integration_tests/version/version_test.go.
There's an issue tracking this, but there's probably not much value in reporting it as a skip.
If renv is available (as it is on Metworx), we skip the TestLibraryRenv subtest that checks an error when renv is unavailable. This is the one subtest name that makes it through to the scorecard because it's skipped. Reword the subtest to make it a bit shorter and remove the test ID. (In general, we should go through the test suite in remove the test IDs, which are no longer relevant under the current validation approach, but this was the only test ID that made it to the scorecard.)
The version can by overridden at build time TestVersion shouldn't assume any particular output. Reduce this to a smoke tests that checks for a non-empty output.
Generating the docs will be handled by a dedicated executable.
The debug and experiment subcommands are internal debugging tools. Mark them as hidden so that they aren't listed as available commands and aren't skipped by cobra.GenMarkdownTree (which will be used in an upcoming commit to write a directory of documentation files).
run: ./scripts/run-integration-tests | ||
env: | ||
PKGR_TESTS_SYS_RENV: ${{ matrix.config.renv && '1' || '' }} | ||
release: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same approach as bbi:
This release job hasn't been tested. My thinking is that we can do a prerelease after the validation rework lands, and I can adjust as needed then.
Flagged by golangci-lint: shadow: declaration of "localpath" shadows declaration at line 98 (govet)
This is the result of $ go get -u golang.org/x/tools $ go mod tidy The x/tools update is required to get a version that defines cover.ParseProfilesFromReader.
This is the result of 'make vt-gen-docs'.
This includes all the test files listed in the previous validation docs except for integration_tests/baseline/root_test.go. That tested the --version option (which is covered by integration_tests/version). If we want to list the test in the traceability matrix, we could include it under the top-level pkgr entry (which would seem a bit odd given that's the entry point to everything) or introduce a version subcommand. But leave it be for now.
With the go.mod update in this series, building pkgr on Drone is now failing with several error messages that say //go:build comment without // +build comment Based on a quick search, it seems like a bug with Go 1.16 (see golang's issue 51436). That lines up with the Go version we pull in to build the binary and the Go version bundled with the r-dev-ci images. We were planning to switch to GitHub Actions soon, but, with the above Drone error, it makes sense to do with this series. The Drone build primarily tested on r-dev-ci:4.1.0. The exceptions were * r-dev-ci:4.0.5 for the mixed-source tests * r-dev-ci-mpn-4.1:2022-02-11 and r-dev-ci-mpn-4.1:cran-latest for the renv library checks For the first point, using 4.0.5 is no longer necessary now that TestMixedSource generates its own repo. Under GitHub Actions, handle the 'renv vs no renv' by setting up two different jobs, one with a system-wide renv installation and one without. Unlike Drone, this leaves out testing an renv version before 0.15 [*], which should be okay given the renv 0.15 release is now over two years old. Note that the library for renv is set up via Rprofile.site rather than communicated through an environment variable (e.g., R_LIBS_USER or R_LIBS_SITE) because getRenvLibrary uses RunRscriptWithArgs, which intentionally discards these. (getRenvLibrary should probably be adjusted to avoid that behavior.) For the OS, hold off on adding a job for the latest Ubuntu because that triggers several 'pkgr install' tests to fail due to a fansi build error. [*] See 0a4a8ee (ci: test configlib and integration_tests/library with system renv, 2022-03-08) and f5c120d (config: invoke renv to discover library path, 2022-01-21) for why testing an renv version before 0.15 was of interest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks great. As usual, I appreciate the documentation in the commit messages. The README in the subtree (at internal/valtools/README.md
) is also quite helpful.
I found two small typos in that README, but I'm approving this now so that it can be merged through once those are fixed upstream and pulled in. Thanks for all the work on this.
internal/valtools/README.md
Outdated
|
||
`docs/commands/` is taken as the directory for the command | ||
documentation unless the `VT_DOC_DIR` variable specifies another | ||
locatino (see [step 5](#step5)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo s/locatino/location/
internal/valtools/README.md
Outdated
### 5. Wire up Makefile | ||
|
||
To wire up the subtree to the main repository, include the subtree's | ||
`rule.mk` file in the repository's top-level Makefile. Before the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: s/rule.mk/rules.mk
(I think?)
@@ -0,0 +1,93 @@ | |||
name: CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the PR description:
Given the plan to switch to GitHub Actions soon, I'd rather do that now than put effort into updating the images for Drone.
This makes sense to me.
Thanks to Seth for spotting.
Thanks. Fixed those (and ran through spell check to find a few more :/) |
This series wires up pkgr to produce external scores for mpn.scorecard. Much of the work is done by the new
internal/valtools/
subtree, which has shared tooling that will also be used by bbi. To set that up on pkgr's side, we need todefine a traceability matrix
add a command to generate markdown pages for the CLI
add scripts for running the tests
internal/valtools/README.md
has more details.With these changes, mpn.scorecard input can be generated by running
make vt-all
from the top-level directory.The goal of this PR is to get all the setup in place that we need for generating the scores, leaving additional changes and cleanup for we want for the scorecard (e.g., fleshing out documentation and adding missing tests) to followup PRs.
GitHub Actions
The goal of this PR was not to switch to GitHub Actions. Unfortunately, something about the dependency updates triggers what looks to be a Go 1.16 bug (see actions commit message for a few more details). Given the plan to switch to GitHub Actions soon, I'd rather do that now than put effort into updating the images for Drone.
Overview
Here's an overview of the commits in the first-parent line.
These are various test tweaks (e.g., related to skipping, which make it through to the scorecard).
These two are prep commits related to generating the command documentation under
docs/commands/
.This is the merge commit that brings in the shared tooling.
These are focused on hooking up the
internal/valtools/
subtree.