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

Introduce _id keyword for manually giving testitems a stable identifer #89

Merged
merged 12 commits into from
Jun 29, 2023

Conversation

nickrobinson251
Copy link
Collaborator

if the testitem name is being automatically set to something liable to change, e.g. file:line location of the testitem, then for the sake of reporting you might want to automatically attach an identifier that's less liable to change (while still keeping the more informative name for the sake of logging).

Part of https://relationalai.atlassian.net/jira/software/c/projects/RAI/issues/RAI-13793

Copy link
Collaborator

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

So is the idea here users can provide an id="XYZ" keyword arg to uniquely identify, but they don't have to provide a name? Are we doing any checking that the provided id is actually unique vs. all other parsed test items?

@nickrobinson251
Copy link
Collaborator Author

currently, testitems must have a name, and always have a file, and for the same file every name must be unique... so file/name is a unique identifier... and relatively stable (people don't change the name of a test, or move tests between files very often)

But, if you know that you're setting a name that is very liable to change, e.g. you're setting it to be the line number, then we provide the option of passing a id keyword as well as a name. That way, you can take control of setting a more stable identifier, without having to change the name. It's super niche to our internal use-case:
the @test_rel (unfortunately) doesn't require a name, so when turning it into a @testitem we're automatically setting the name to as file:line e.g. foo_test.jl:42, we want this name to be somewhat informative, hence picking file:line, but we also want a way to track tests over time (and line numbers change very often, especially when we have files that have hundreds of tests, and are not append-only), so we're thinking of setting a separate id that's a hash of the test's actual contents (which should not change often): https://github.com/RelationalAI/raicode/pull/14338

Copy link
Collaborator

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

Sounds good; thanks @nickrobinson251! I wonder if we even call it something like _id just to try and emphasize that this really shouldn't be needed in the non-us case.

function check_ids(testitems)
ids = getproperty.(testitems, :id)
# This should only be possible to trip if users are manually passing the `_id` keyword.
allunique(ids) || _throw_duplicate_ids(testitems)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are we happy throwing on this? I suppose hash collisions are possible, right?

@nickrobinson251 nickrobinson251 marked this pull request as ready for review June 28, 2023 15:07
@nickrobinson251 nickrobinson251 changed the title Introduce id keyword for manually giving testitems setting a stable identifer Introduce id keyword for manually giving testitems a stable identifer Jun 28, 2023
@nickrobinson251 nickrobinson251 changed the title Introduce id keyword for manually giving testitems a stable identifer Introduce _id keyword for manually giving testitems a stable identifer Jun 29, 2023
@nickrobinson251 nickrobinson251 enabled auto-merge (squash) June 29, 2023 11:57
@nickrobinson251 nickrobinson251 merged commit 3e73335 into main Jun 29, 2023
@nickrobinson251 nickrobinson251 deleted the npr-stable-id branch June 29, 2023 12:06
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