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

moving towards sourmash v5, and building out our test infrastructure to support it #3076

Open
ctb opened this issue Mar 11, 2024 · 1 comment
Labels
5.0 issues to address for a 5.0 release

Comments

@ctb
Copy link
Contributor

ctb commented Mar 11, 2024

PR #3072 is a proposed step along the road to support a sourmash v5 release 🎉. PR #3074 tries the same test infrastructure out on a second fix.

Specifically, #3072 adds support for --v4 and --v5 command-line arguments to sig check and sig collect, along with test fixtures and a version=... argument to runtmp.sourmash. --v4 would retain the current default v4 behavior, while --v5 would switch the behavior over to the new, exciting (proposed) sourmash v5 behavior.

The goal is to support the incremental development and testing of sourmash v5 behavior, rather than needing to do a big switchover all at once.

The PR adds three fixtures: cli_v4_and_v5, cli_v4_only, and cli_v5_only. Right now the fixtures provide various mixtures of [default/no version specified, --v4, --v5] behavior.

The idea is that you can set up a test like this:

def test_something(runtmp, ..., cli_v4_and_v5):
    ...
    runtmp.sourmash('sig', 'check', ..., version=cli_v4_and_v5)

and get --v4 and --v5 added to the CLI, along with a "no version specified", i.e. default behavior.

This addresses three specific use cases:

  • we have default behavior that's not going to change. we can just leave that alone, or provide tests that run both --v4 and --v5.
  • we have behavior that is default in v4, but is going to change in v5. We can test that with --v4 and (default) now, and then remove (default) from the fixture later.
  • we have behavior that is going to be default in v5. We can test that with --v5 now, and then add (default) later when we release v5.

One benefit of this approach: we can test the switch to default behavior by changing only two items in conftest.py!

Another nice feature is that we can switch a whole test (potentially with multiple sourmash commands in it) over to the new version during the test, using the decorators.

Note that we will still need to write a bunch of tests for v5 behavior specifically, of course.

Questions and thoughts:

  • Do we want to change the test infra to run --v4 by default? Probably not, except as a test.
  • do we want to support --v4 for any length of time after sourmash v5 is released?
    • Hot take: we should transition away from it the moment sourmash v5 release candidates are available, and mostly support --v4 as an explicit alternative to --v5; when --v5 goes away, so does --v4.
    • (Although maybe we want to leave --v5 as a no-op in sourmash v5, to help prepare for --v6? ;)
  • do we want to add --v4 and --v5 support to everything, even when they are ignored? Or only those commands where we are changing some behavior?
  • Hot take: I think adding them to everything is better UX behavior so that script/workflow users can just add --v4 to everything, and slowly change over to --v5 as they refactor, but I'm not committed to that!
  • It's not entirely clear what the combined fixture cli_v4_and_v5 gives us other than ensuring that both --v4 and --v5 are available 🤔 . I guess it tests that the version switch code is working properly for behavior that is staying the same? But that would be discovered anyway when we switch things over. It could also be used to explicitly mark that the behavior for this test is the same.

Alternatives:

  • Instead of using param fixtures, we could make a runtmp_v5 fixture that would automatically add --v5 on to things. However, that wouldn't give us the combinatorics that we get with fixtures, though, where we get to test with multiple --v4, --v5, and default behavior.

A process for v5 testing

  • identify specific tests that require v4 behavior, and write new ones for v5 behavior
  • implement the necessary v4/v5 switches in the code, verify they work with the right fixtures
  • catch other tests that are affected by enabling the v5 behavior by default in the code
@ctb ctb added the 5.0 issues to address for a 5.0 release label Mar 11, 2024
@ctb
Copy link
Contributor Author

ctb commented Mar 11, 2024

curious what @sourmash-bio/devs think :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 issues to address for a 5.0 release
Projects
None yet
Development

No branches or pull requests

1 participant