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

Simplify baseline names in snapshot testing #105

Merged
merged 2 commits into from
Feb 24, 2025

Conversation

bbannier
Copy link
Member

We previously would include a running number for the specific test
parameterization in the snapshot for each sample test. This meant that
tests could get renamed whenever a new sample was added.

With this patch we now override the default name enumerating each
parameterization and instead set it to the name of the sample a test
runs on. With that snapshots should be much more stable.

@bbannier bbannier self-assigned this Feb 24, 2025
@bbannier bbannier marked this pull request as ready for review February 24, 2025 14:32
@bbannier bbannier requested a review from awelzel February 24, 2025 14:32
Copy link

@awelzel awelzel left a comment

Choose a reason for hiding this comment

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

LGTM, only thought is if _get_samples() could return something like:

{
    "argvalues": [ ... ],
    "ids": [....],
}

And then **-splat it in the parameterize() call. Haven't tried, but that would make the naming/ordering local to _get_samples().

@bbannier
Copy link
Member Author

{
"argvalues": [ ... ],
"ids": [....],
}


And then `**-splat` it in the parameterize() call. Haven't tried, but that would make the naming/ordering local to `_get_samples()`.

Purely from a typing standpoint, we'd need to return a similar but empty dict on the exception path for things to align, e.g.,

class Samples(TypedDict):
    argvalues: list[Path]
    ids: list[str]


def _get_samples() -> Samples:
   # ...
   try:
      # ...
        return {
            "argvalues": samples,
            "ids": [str(os.path.basename(s)) for s in samples],
        }
    except FileNotFoundError:
        return {"argvalues": [], "ids": []}

Would that look reasonable to you?

@bbannier bbannier force-pushed the topic/bbannier/simplify-baselines branch from 2807210 to c3ed33e Compare February 24, 2025 15:30
@awelzel
Copy link

awelzel commented Feb 24, 2025

Would that look reasonable to you?

Could we ditch FileNotFound error? Most likely empty samples is not what one would want to test anyhow?

I was thinking more about the following, but I'm now thinking your initial proposal with using ids= explicitly is better and easier to understand. Sorry for the noise.

def _get_samples():
    """Helper to enumerate samples"""

    # We exclude directories since we store snapshots along with the samples.
    # This assumes that there are no tests in subdirectories of `SAMPLES_DIR`.
    samples, ids = [], []
    for s in sorted([sample for sample in SAMPLES_DIR.iterdir() if sample.is_file]):
        samples += [s]
        ids += [s.parts[-1]]

    return {
        "argvalues": samples,
        "ids": ids,
    }

We previously would include a running number for the specific test
parameterization in the snapshot for each sample test. This meant that
tests could get renamed whenever a new sample was added.

With this patch we now override the default name enumerating each
parameterization and instead set it to the name of the sample a test
runs on. With that snapshots should be much more stable.
@bbannier bbannier force-pushed the topic/bbannier/simplify-baselines branch from c3ed33e to aabb000 Compare February 24, 2025 16:38
@bbannier
Copy link
Member Author

Removed the handling of FileNotFound which can only be raised if SAMPLES_DIR does not exist, inserted an assertion instead.

I agree that the coupling between the internals of _get_samples (in particular: returns a sorted list), and its use is not nice, but like you wrote pulling all that into _get_samples itself doesn't make things much simpler either.

@bbannier bbannier merged commit aede394 into main Feb 24, 2025
7 checks passed
@bbannier bbannier deleted the topic/bbannier/simplify-baselines branch February 24, 2025 17:25
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