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

Add unit tests for some reproducibility infrastructure #3437

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

charleskawczynski
Copy link
Member

This PR adds some unit tests for the reproducibility infrastructure, and attempts to improve the documentation.

Thanks for suggesting this, @Sbozzolo.

One downside of this PR is that I've removed this check:

    ref_counters_main = map(read_ref_counter, ref_counter_files_main)
    if all(rc -> ref_counter_PR == rc + 1, ref_counters_main) # new reference
        @warn "`ref_counter.jl` incremented, assuming no comparable references"
        @info "Ref counters main: $ref_counters_main."
        @info "Please review output results before merging."
        return String[]
    elseif all(rc -> ref_counter_PR == rc, ref_counters_main) # unchanged reference
        @info "Ref counters main: $ref_counters_main."
        @info "Comparing results against main path:$paths"
    else
        error(
            "Unexpected reference. Please open an issue pointing to this build.",
        )
    end

But I think it no longer makes sense in the context of latest_comparable_paths returning comparable paths, because the path must contain the same ref counter as the main branch (which is now specified as a function argument).

Am I missing any edge cases that could happen?

@charleskawczynski
Copy link
Member Author

I was going to add to this, but I think I'd rather merge this in smaller pieces.

@charleskawczynski charleskawczynski merged commit 68480cf into main Nov 14, 2024
16 checks passed
@charleskawczynski charleskawczynski deleted the ck/repro_refactor branch November 14, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant