Skip to content

Split test history code into separate module - #194

Merged
christiangnrd merged 2 commits into
mainfrom
splithist
Sep 21, 2026
Merged

christiangnrd merged 2 commits into
mainfrom
splithist

Conversation

@christiangnrd

@christiangnrd christiangnrd commented Sep 20, 2026 •

Copy link
Copy Markdown
Collaborator

(DO NOT SQUASH)

Also fix test that passed on retry being recorded as a failure.

@christiangnrd
christiangnrd force-pushed the splithist branch 2 times, most recently from 85bbe23 to 8eb3eca Compare September 20, 2026 21:37
@christiangnrd
christiangnrd added this pull request to stack #196 September 20, 2026 21:39

@giordano giordano left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for fixing the bug, 100% my fault, I didn't consider retries at all in #188. Can you please bump the patch version?

@giordano giordano left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry, I'm going to be pesky about style nits

Comment thread src/history.jl Outdated
Comment on lines +2 to +4
using Scratch
using Serialization
using FileWatching: Pidfile

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not a fan of indenting inside module

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Comment thread src/history.jl Outdated
using Serialization
using FileWatching: Pidfile

import ..ParallelTestRunner: AbstractTestRecord, anynonpass, Lockable

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also, not a fan at all of import, it's spooky action at a distance.

Suggested change
import ..ParallelTestRunner: AbstractTestRecord, anynonpass, Lockable
using ..ParallelTestRunner: ParallelTestRunner

or

Suggested change
import ..ParallelTestRunner: AbstractTestRecord, anynonpass, Lockable
import ..ParallelTestRunner

if you prefer (I'm just allergic to the import keyword), and explicitly qualify method extensions below

@christiangnrd christiangnrd Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I went with

import ..ParallelTestRunner as PTR

but if you'd rather I do one of your suggestions I'm happy to change it

christiangnrd and others added 2 commits September 20, 2026 22:12
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@christiangnrd
christiangnrd merged commit 0be4b81 into main Sep 21, 2026
18 checks passed
@christiangnrd
christiangnrd deleted the splithist branch September 21, 2026 01:28
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