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’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

Merged
merged 43 commits into from
Aug 13, 2024
Merged

Set up valtools subtree and move to GitHub Actions #419

merged 43 commits into from
Aug 13, 2024

Conversation

kyleam
Copy link
Collaborator

@kyleam kyleam commented Aug 13, 2024

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 to

  • define 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.

[01] rcmd/configure_test.go: prevent tempdir check from skipping
[02] integration_tests/baseline: delete root_test.go
[03] integration_tests/mixed-source: drop skip for inactive test
[04] integration_tests/library: fix skip message typo
[05] integration_tests/library: shorten a subtest name
[06] integration_tests/version: loosen output check

These are various test tweaks (e.g., related to skipping, which make it through to the scorecard).

[07] cmd/pkgr: delete commented code for generating docs
[08] cmd: hide debug and experiment subcommands

These two are prep commits related to generating the command documentation under docs/commands/.

[09] add internal/valtools/ subtree

This is the merge commit that brings in the shared tooling.

[10] go.mod: update for internal/valtools addition
[11] internal/tools: define executable for generating command docs
[12] docs: generate command docs
[13] add script for running unit tests
[14] add script for running integration tests
[15] make: wire up internal/valtools
[16] docs: define a traceability matrix

These are focused on hooking up the internal/valtools/ subtree.

[17] ci: switch to GitHub Actions

kyleam added 29 commits July 31, 2024 09:32
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:
Copy link
Collaborator Author

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.

kyleam added 12 commits August 13, 2024 10:29
Flagged by golangci-lint:

  shadow: declaration of "localpath" shadows declaration at line 98 (govet)
git-subtree-dir: internal/valtools
git-subtree-mainline: 292c31b
git-subtree-split: 928bcb2
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.
@kyleam kyleam requested a review from seth127 August 13, 2024 14:46
@seth127 seth127 mentioned this pull request Aug 13, 2024
Base automatically changed from test-fixes to develop August 13, 2024 19:26
Copy link
Collaborator

@seth127 seth127 left a 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.


`docs/commands/` is taken as the directory for the command
documentation unless the `VT_DOC_DIR` variable specifies another
locatino (see [step 5](#step5)).
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo s/locatino/location/

### 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
Copy link
Collaborator

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
Copy link
Collaborator

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.

@kyleam
Copy link
Collaborator Author

kyleam commented Aug 13, 2024

I found two small typos in that README,

Thanks. Fixed those (and ran through spell check to find a few more :/)

@kyleam kyleam merged commit ca1712c into develop Aug 13, 2024
3 checks passed
@kyleam kyleam deleted the valtools branch August 13, 2024 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants