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.

Sign up for GitHub

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

Jump to bottom

Test: add option to record passes and enable it for CI #57690

Closed
wants to merge 2 commits into from
Closed

Test: add option to record passes and enable it for CI #57690

wants to merge 2 commits into from

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Mar 9, 2025 •
edited
Loading

Related to #57686

Investigating how feasible it would be to record test passes in full so they can be used by buildkite analytics. They are currently just counted.

Todo

  • There's some dummy recording going on that needs removing if this is enabled

    julia/test/runtests.jl

    Lines 381 to 395 in 0b9525b

    elseif isa(resp, Test.TestSetException)
    fake = Test.DefaultTestSet(testname)
    fake.time_end = fake.time_start + duration
    for i in 1:resp.pass
    Test.record(fake, Test.Pass(:test, nothing, nothing, nothing, LineNumberNode(@__LINE__, @__FILE__)))
    end
    for i in 1:resp.broken
    Test.record(fake, Test.Broken(:test, nothing))
    end
    for t in resp.errors_and_fails
    Test.record(fake, t)
    end
    Test.push_testset(fake)
    Test.record(o_ts, fake)
    Test.pop_testset()
  • Figure out why the weakref test is failing. I think the Pass object might be retaining a reference?

@IanButterworth IanButterworth added testsystem The unit testing framework and Test stdlib ci Continuous integration labels Mar 9, 2025
@IanButterworth IanButterworth mentioned this pull request Mar 9, 2025
Merged
Seelengrab
Seelengrab reviewed Mar 9, 2025
View reviewed changes
stdlib/Test/src/Test.jl
@@ -1101,7 +1105,11 @@ struct FailFastError <: Exception end
# For a broken result, simply store the result
record(ts::DefaultTestSet, t::Broken) = (push!(ts.results, t); t)
# For a passed result, do not store the result since it uses a lot of memory
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# For a passed result, do not store the result since it uses a lot of memory

This comment should be removed/changed to mention that this only happens conditionally with this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Yeah this is more of an investigation of the performance/test side effect implications currently.

Seelengrab
Seelengrab reviewed Mar 9, 2025
View reviewed changes
test/testdefs.jl
Test.TESTSET_PRINT_ENABLE[] = false
ENV["JULIA_TEST_RECORD_PASSES"] = Base.get_bool_env("CI", false)
Copy link
Contributor

Choose a reason for hiding this comment

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

withenv?

@KristofferC
Copy link
Member

For some history: #20137 + #20150

@DilumAluthge
Copy link
Member

Instead of keeping all the successful tests in memory, could we stream them to a file?

This would be off by default, and we'd turn it on only in Buildkite CI.

@IanButterworth
Copy link
Member Author

Replaced by #57686

@IanButterworth IanButterworth deleted the ib/test_record_passes branch March 19, 2025 04:30
IanButterworth added a commit that referenced this pull request Mar 20, 2025
@IanButterworth
CI: Store Passes in results.json + various improvements. (#57686)
d971131 
- Adds option to enable saving Test.Pass results to DefaultTestSet
(started as #57690 but more
complete here) and enable it when Base.runtests is called when CI=true,
to match when reports are saved
- Stops attributing testset duration to tests from that testset, it
doesn't make sense.
- Make the test name resemble (as far as possible) the original test
call. Making the name independent of test outcome should make it easier
to group tests and identify flaky ones.
- store repeat counts as tags, rather than in the name
- moves the save to inside the test workers, where all test info is
available, and the save can be done in parallel to save time , then
jsons are made at the end and uploaded as an artifact in a tarball
- speed improvements to make saving time acceptable
- fixes a couple of tests that seem to get a little flakier with this

Note on the WeakRef test fix: If a test like `@test x == y` passes, the
`Pass` object retains `x`. Previously to this the Pass object was still
made, but must have been GC-ed immediately. This PR remakes the Pass
object when appending it to DefaultTestSet without the captured data. I
think that just delayed the GC a little to make the test flaky, so I
moved the `@test` to outside the function to avoid capturing `x`.

Also buildkite had to fix an issue to make tests show up properly on our
jobs: https://forum.buildkite.community/t/test-failures-showing-up-in-test-digest-but-not-on-jobs/4306
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Seelengrab Seelengrab Seelengrab left review comments

Assignees
No one assigned
Labels
ci Continuous integration testsystem The unit testing framework and Test stdlib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@IanButterworth @KristofferC @DilumAluthge @Seelengrab