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

CI: Tweaks to results.json #57686

Draft
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Mar 8, 2025

  • Stop 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
  • Includes Test: add option to record passes and enable it for CI #57690 for testing purposes

Todo:

  • Determine if overhead is acceptable
  • Remove timing prints
  • Decide whether to merge this with Test: add option to record passes and enable it for CI #57690 or do that separately
  • Figure out why the weakref test is failing. I think the Pass object might be retaining a reference?
    Identified: If a test like @test x == y passes, the Pass object retains x. I changed the weakref test to not do this by testing the outer function, as I don't think it's what that test is actually testing. But the question remains about whether this is unexpected behavior. Might alone be grounds for more eagerly streaming results to file so we can free the referenced values.
  • add stdlib etc prefix to path to make repo links work
  • consolidate/compress the jsons before uploading as a artifacts (250 artifacts is too many artifacts)

@IanButterworth IanButterworth added testsystem The unit testing framework and Test stdlib ci Continuous integration labels Mar 8, 2025
@IanButterworth IanButterworth force-pushed the ib/test_json_fixes branch 2 times, most recently from 95328b4 to 96ec737 Compare March 9, 2025 03:33
@IanButterworth IanButterworth marked this pull request as draft March 9, 2025 03:43
@vchuravy
Copy link
Member

Stop attributing testset duration to tests from that testset, it doesn't make sense.

Can you expand on this a little bit?

@IanButterworth
Copy link
Member Author

Currently we report that a test takes the duration of its parent testset. So @test x == 1 could be reported to take like 10 minutes. I think it's misleading.

@vchuravy
Copy link
Member

Oh yeah, that's silly. Can we capture useful duration times?

@IanButterworth
Copy link
Member Author

IanButterworth commented Mar 10, 2025

Our Result types don't currently store timing information. We could add it, but tbh I'm not sure it's any more useful. The useful information is somewhere inbetween the testset and the @test duration. If testsets were strictly a single test setup + test then it'd probably be fair to report testset duration. But testsets are very commonly a group of many sequential setups and tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration testsystem The unit testing framework and Test stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants