Skip to content

Convoluted hook testing infra #552

Open
@lorenzwalthert

Description

@lorenzwalthert

Is your feature request related to a problem? Please describe.
Convoluted testing infra

Describe the solution you'd like

In general, I think run_test is very convoluted, since std_err (as documented in run_test + me reading the source code) can be

  • a string to signal we expect a certain error message (asserted with matching std err against the expected error message in case).
  • NA to signals we don't expect the file passed to the hook test to change (asserted with comparing file contents before and after).
  • NULL (the default) as a shortcut to avoid typing out expect_success since usually when you expect something in std err, you probably expect the hook to fail. The signature for the reader's convenience:
    run_test(
      std_err = NULL,
      expect_success = is.null(std_err),
      ...
    )

I think to untangle this, we first have to fix run_test_impl() and reduce cognitive overload / multiple meaning on the arguments std_err and std_out :

  • use std_err and std_out only for message matching
  • leverage read_only argument recently added to decide if file content should be the same or not (instead of relying std_err = NA).
  • This might be enough keep the std_err = NULL shortcut without being too confusing.

Describe alternatives you've considered

Not sleep anymore at night since testing infra is messed up 😸

Additional context

Came up in https://github.com/lorenzwalthert/precommit/pull/551/files/b8f72d26d158d86eee8aad887db05ea06f622399#r1541574615

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions