Skip to content

Commit 80b31a5

Browse files
committed
Apply P0 and P1 philosophy fixes: typed exceptions and bug fixes
P0 - Bug Fixes: - TestRunner auto-discovery: Fixed non-recursive test discovery (ext/TestRunner.jl:614) Now uses _collect_test_files_recursive instead of flat readdir P1 - Typed Exceptions (Tenet 6): Replaced all untyped error() and ArgumentError with structured CTBase exceptions P1a - src/Exceptions/types.jl: - Fixed 2 occurrences of invalid Julia syntax catch e::CTBase.Exceptions.CTException → catch e + e isa ... || rethrow() P1b - ext/TestRunner.jl (7 replacements): - 3 × error() in _resolve_test → CTBase.Exceptions.IncorrectArgument - 1 × error() in _run_single_test (function missing after include) → PreconditionError - 1 × error() in run_tests (subdirectory "test" forbidden) → PreconditionError - 2 × ArgumentError in _normalize_available_tests → IncorrectArgument P1c - ext/CoveragePostprocessing.jl (4 replacements): - 4 × error() → PreconditionError P1d - ext/DocumenterReference.jl (2 replacements): - public && private both false → IncorrectArgument - Invalid element in primary_modules → IncorrectArgument P1e - ext/TestRunner.jl docstring: - Fixed incorrect output in _progress_bar docstring (width=20 → 10 blocks, not 20) P1f - Test updates: - Updated 7 test files to reflect new exception types Code cleanup: - Removed circular import using CTBase from src/Exceptions/Exceptions.jl - Removed redundant using DocStringExtensions from src/Descriptions/types.jl All tests pass: 1161/1161
1 parent 031aa03 commit 80b31a5

10 files changed

Lines changed: 85 additions & 143 deletions

File tree

ext/CoveragePostprocessing.jl

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -88,31 +88,28 @@ function CTBase.postprocess_coverage(
8888

8989
n_cov = _count_cov_files(source_dirs)
9090
if n_cov == 0
91-
error("""
92-
Coverage requested but no .cov files were produced.
93-
94-
Expected locations:
95-
- src/*.cov
96-
- test/*.cov
97-
- ext/*.cov
98-
99-
Did you run:
100-
Pkg.test(...; coverage=true) ?
101-
""")
91+
throw(CTBase.Exceptions.PreconditionError(
92+
"Coverage requested but no .cov files were produced",
93+
reason="no .cov files found in src/, test/, or ext/",
94+
suggestion="run Pkg.test(...; coverage=true) to generate coverage data",
95+
))
10296
end
10397

10498
_clean_stale_cov_files!(source_dirs)
10599

106100
n_cov = _count_cov_files(source_dirs)
107-
n_cov == 0 &&
108-
error("Coverage requested but no usable .cov files were found after cleanup.")
101+
n_cov == 0 && throw(CTBase.Exceptions.PreconditionError(
102+
"Coverage requested but no usable .cov files were found after cleanup",
103+
))
109104

110105
generate_report && _generate_coverage_reports!(
111106
source_dirs, coverage_dir, root_dir, worst_n_files, max_uncovered_lines
112107
)
113108

114109
moved = _collect_and_move_cov_files!(source_dirs, cov_storage_dir)
115-
isempty(moved) && error("Coverage requested but no .cov files were found to move.")
110+
isempty(moved) && throw(CTBase.Exceptions.PreconditionError(
111+
"Coverage requested but no .cov files were found to move",
112+
))
116113
println("✓ Moved $(length(moved)) .cov files to $cov_storage_dir")
117114

118115
println("✓ Coverage post-processing completed successfully")
@@ -295,7 +292,9 @@ function _generate_coverage_reports!(
295292
append!(cov_all, Coverage.process_folder(dir))
296293
end
297294

298-
isempty(cov_all) && error("No coverage data found in source directories")
295+
isempty(cov_all) && throw(CTBase.Exceptions.PreconditionError(
296+
"No coverage data found in source directories",
297+
))
299298

300299
lcov_file = joinpath(coverage_dir, "lcov.info")
301300
Coverage.LCOV.writefile(lcov_file, cov_all)

ext/DocumenterReference.jl

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -233,9 +233,10 @@ function CTBase.automatic_reference_documentation(
233233
)
234234
# Validate arguments
235235
if !public && !private
236-
error(
237-
"automatic_reference_documentation: both `public` and `private` cannot be false.",
238-
)
236+
throw(CTBase.Exceptions.IncorrectArgument(
237+
"both `public` and `private` cannot be false",
238+
context="automatic_reference_documentation",
239+
))
239240
end
240241

241242
# Parse primary_modules into a Dict{Module, Vector{String}}
@@ -387,9 +388,11 @@ function _parse_primary_modules(primary_modules::Vector)
387388
files = last(m)
388389
result[mod] = _normalize_paths(files isa Vector ? files : [files])
389390
else
390-
error(
391+
throw(CTBase.Exceptions.IncorrectArgument(
391392
"Invalid element in primary_modules: expected Module or Module => files pair",
392-
)
393+
got=string(typeof(m)),
394+
context="_parse_primary_modules",
395+
))
393396
end
394397
end
395398
return result

ext/TestRunner.jl

Lines changed: 39 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -154,12 +154,11 @@ function CTBase.run_tests(
154154
# Guard: a subdirectory named "test" inside test_dir would conflict with
155155
# the automatic `test/` prefix stripping in _parse_test_args.
156156
if isdir(joinpath(test_dir, "test"))
157-
error(
158-
"A subdirectory \"test\" exists inside the test directory " *
159-
"\"$(test_dir)\". This is not supported because selection " *
160-
"arguments starting with \"test/\" are automatically stripped " *
161-
"(e.g. \"test/suite\"\"suite\"). Please rename the subdirectory.",
162-
)
157+
throw(CTBase.Exceptions.PreconditionError(
158+
"A subdirectory \"test\" exists inside the test directory \"$(test_dir)\"",
159+
reason="selection arguments starting with \"test/\" are automatically stripped (e.g. \"test/suite\"\"suite\")",
160+
suggestion="rename the subdirectory to avoid the conflict",
161+
))
163162
end
164163

165164
# Parse command-line arguments
@@ -194,7 +193,6 @@ function CTBase.run_tests(
194193
@testset "$(spec)" begin
195194
_run_single_test(
196195
spec;
197-
available_tests=available_tests_vec,
198196
filename_builder,
199197
funcname_builder,
200198
eval_mode,
@@ -480,15 +478,21 @@ function _normalize_available_tests(available_tests)
480478
available_tests === nothing && return TestSpec[]
481479

482480
if !(available_tests isa AbstractVector || available_tests isa Tuple)
483-
throw(ArgumentError("available_tests must be a Vector or Tuple of Symbol/String"))
481+
throw(CTBase.Exceptions.IncorrectArgument(
482+
"available_tests must be a Vector or Tuple of Symbol/String",
483+
got=string(typeof(available_tests)),
484+
))
484485
end
485486

486487
out = TestSpec[]
487488
for entry in available_tests
488489
if entry isa Symbol || entry isa String
489490
push!(out, entry)
490491
else
491-
throw(ArgumentError("available_tests entries must be Symbol or String"))
492+
throw(CTBase.Exceptions.IncorrectArgument(
493+
"available_tests entries must be Symbol or String",
494+
got=string(typeof(entry)),
495+
))
492496
end
493497
end
494498
return out
@@ -609,47 +613,11 @@ function _select_tests(
609613
filename_builder::Function;
610614
test_dir::String=joinpath(pwd(), "test"), # Default assumption
611615
)
612-
# 1. Identify all potential test files
613-
# We look for .jl files in test_dir, excluding runtests.jl
614-
all_files = filter(f -> endswith(f, ".jl") && f != "runtests.jl", readdir(test_dir))
615-
616-
# Map filenames back to "test names" is tricky without reverse builder.
617-
# Strategy:
618-
# We assume `available_tests` defines the canonical list of "names".
619-
# If `available_tests` is empty, we derive names from files?
620-
# -> User said: "list all .jl files... but only keep available if provided"
621-
622616
candidates = TestSpec[]
623617

624618
if isempty(available_tests)
625-
# If no available_tests provided, every .jl file is a candidate!
626-
# This effectively AUTO-DISCOVERS tests.
627-
# We need to guess the "name" from the filename.
628-
# This is hard because filename_builder is name -> filename.
629-
# utils -> test_utils.jl
630-
# test_utils.jl -> utils ??
631-
# For now, let's keep the user's current behavior:
632-
# If available_tests is empty, the logic relies on what the user passes.
633-
# BUT the new logic says "if args empty -> run all".
634-
# So we MUST perform auto-discovery if we want "run all" to work without explicit available_tests.
635-
636-
# Heuristic: if file starts with "test_", strip it?
637-
# Or just use the basename as the symbol?
638-
# Let's assume the "name" is the filename without extension for auto-discovery?
639-
# Or better: don't guess names yet. Just work with filenames?
640-
# But `run_tests` iterates over `names`.
641-
642-
# Let's look at existing files: test_utils.jl -> name=:utils (via filename_builder)
643-
# We cannot invert `filename_builder` (F -> S).
644-
# So we are stuck if we want to infer names from files.
645-
646-
# COMPROMISE: If available_tests is empty, we cannot guarantee auto-discovery compatibility with arbitrary filename_builders.
647-
# We will assume a default mapping for auto-discovery: name = file_basename_without_extension.
648-
for f in all_files
649-
name_str = replace(f, ".jl" => "")
650-
# If name starts with "test_", removing it is common convention, but maybe risky?
651-
# Let's keep the full basename as the name if we are auto-discovering.
652-
push!(candidates, Symbol(name_str))
619+
for f in _collect_test_files_recursive(test_dir)
620+
push!(candidates, f)
653621
end
654622
else
655623
# If available_tests IS provided, we only consider these.
@@ -764,7 +732,6 @@ This helper:
764732
765733
# Arguments
766734
- `spec::TestSpec`: Test specification to run
767-
- `available_tests::AbstractVector{<:TestSpec}`: Available tests for validation
768735
- `filename_builder::Function`: Function to map test names to filenames
769736
- `funcname_builder::Function`: Function to map test names to function names
770737
- `eval_mode::Bool`: Whether to evaluate the function after include
@@ -780,7 +747,6 @@ This helper:
780747
"""
781748
function _run_single_test(
782749
spec::TestSpec;
783-
available_tests::AbstractVector{<:TestSpec},
784750
filename_builder::Function,
785751
funcname_builder::Function,
786752
eval_mode::Bool,
@@ -792,18 +758,19 @@ function _run_single_test(
792758
)
793759
# --- Resolve filename and func_symbol ---
794760
filename, func_symbol = _resolve_test(
795-
spec; available_tests, filename_builder, funcname_builder, eval_mode, test_dir
761+
spec; filename_builder, funcname_builder, eval_mode, test_dir
796762
)
797763

798764
# --- Include the file ---
799765
Base.include(Main, filename)
800766

801767
# --- Check function exists after include ---
802768
if eval_mode && func_symbol !== nothing && !isdefined(Main, func_symbol)
803-
error("""
804-
Function "$(func_symbol)" not found after including "$(filename)".
805-
Make sure the file defines a function with this name.
806-
""")
769+
throw(CTBase.Exceptions.PreconditionError(
770+
"Function \"$(func_symbol)\" not found after including \"$(filename)\"",
771+
reason="the file does not define a function with this name",
772+
suggestion="make sure the file defines a top-level function named $(func_symbol)",
773+
))
807774
end
808775

809776
# --- on_test_start callback ---
@@ -879,7 +846,6 @@ Raises errors if the file is not found or if `eval_mode=true` but no function ca
879846
880847
# Arguments
881848
- `spec::TestSpec`: Test specification to resolve
882-
- `available_tests::AbstractVector{<:TestSpec}`: Available tests for validation
883849
- `filename_builder::Function`: Function to map test names to filenames
884850
- `funcname_builder::Function`: Function to map test names to function names
885851
- `eval_mode::Bool`: Whether to resolve a function name
@@ -900,7 +866,6 @@ Raises errors if the file is not found or if `eval_mode=true` but no function ca
900866
"""
901867
function _resolve_test(
902868
spec::TestSpec;
903-
available_tests::AbstractVector{<:TestSpec},
904869
filename_builder::Function,
905870
funcname_builder::Function,
906871
eval_mode::Bool,
@@ -910,10 +875,10 @@ function _resolve_test(
910875
rel = _ensure_jl(spec)
911876
filename = joinpath(test_dir, rel)
912877
if !isfile(filename)
913-
error("""
914-
Test file "$(filename)" not found.
915-
Current directory: $(pwd())
916-
""")
878+
throw(CTBase.Exceptions.IncorrectArgument(
879+
"Test file \"$(filename)\" not found",
880+
context="current directory: $(pwd())",
881+
))
917882
end
918883

919884
func_symbol = if eval_mode
@@ -929,30 +894,29 @@ function _resolve_test(
929894

930895
# Build filename
931896
rel = _find_symbol_test_file_rel(name, filename_builder; test_dir=test_dir)
932-
rel === nothing && error("""
933-
Test file not found for test "$(name)".
934-
Current directory: $(pwd())
935-
""")
897+
rel === nothing && throw(CTBase.Exceptions.IncorrectArgument(
898+
"Test file not found for test \"$(name)\"",
899+
context="current directory: $(pwd())",
900+
))
936901

937902
filename = joinpath(test_dir, rel)
938903

939904
# Check file exists
940-
!isfile(filename) && error("""
941-
Test file "$(filename)" not found for test "$(name)".
942-
Current directory: $(pwd())
943-
""")
905+
!isfile(filename) && throw(CTBase.Exceptions.IncorrectArgument(
906+
"Test file \"$(filename)\" not found for test \"$(name)\"",
907+
context="current directory: $(pwd())",
908+
))
944909

945910
# Determine function name
946911
func_symbol = funcname_builder(name)
947912

948913
# Check consistency: eval_mode=true but funcname_builder returns nothing
949914
if eval_mode && func_symbol === nothing
950-
error(
951-
"""
952-
Inconsistency: eval_mode=true but funcname_builder returned nothing for test "$(name)".
953-
Either set eval_mode=false, or make funcname_builder return a Symbol.
954-
""",
955-
)
915+
throw(CTBase.Exceptions.PreconditionError(
916+
"eval_mode=true but funcname_builder returned nothing for test \"$(name)\"",
917+
reason="funcname_builder must return a Symbol when eval_mode=true",
918+
suggestion="set eval_mode=false, or make funcname_builder return a Symbol",
919+
))
956920
end
957921

958922
if !eval_mode || func_symbol === nothing
@@ -1086,7 +1050,7 @@ julia> TestRunner._progress_bar(5, 10)
10861050
"[█████░░░░░]"
10871051
10881052
julia> TestRunner._progress_bar(5, 10; width=20)
1089-
"[████████████████████░░░░░░]"
1053+
"[██████████░░░░░░░░░░]"
10901054
10911055
julia> TestRunner._progress_bar(0, 10; width=5)
10921056
"[░░░░░]"

src/Descriptions/types.jl

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
# Type definitions for Descriptions module
22

3-
using DocStringExtensions
4-
53
"""
64
$(TYPEDEF)
75

src/Exceptions/Exceptions.jl

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,6 @@ The Exceptions module is organized into thematic files:
3636
"""
3737
module Exceptions
3838

39-
using CTBase
40-
4139
# Type definitions
4240
include("types.jl")
4341

src/Exceptions/types.jl

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@ julia> using CTBase
1616
1717
julia> try
1818
throw(CTBase.Exceptions.IncorrectArgument("invalid input"))
19-
catch e::CTBase.Exceptions.CTException
19+
catch e
20+
e isa CTBase.Exceptions.CTException || rethrow()
2021
println("Caught a domain-specific exception: ", e)
2122
end
2223
Caught a domain-specific exception: IncorrectArgument: invalid input
@@ -31,7 +32,8 @@ catching all exceptions of this family via `catch e::CTException`.
3132
try
3233
# code that may throw CTBase exceptions
3334
risky_operation()
34-
catch e::CTBase.Exceptions.CTException
35+
catch e
36+
e isa CTBase.Exceptions.CTException || rethrow()
3537
# handle all CTBase domain errors uniformly
3638
handle_error(e)
3739
end

test/suite/extensions/test_coverage_edge_cases.jl

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,6 @@ function test_coverage_edge_cases()
103103
err = try
104104
TR._run_single_test(
105105
:phantom_test;
106-
available_tests=Symbol[],
107106
filename_builder=identity,
108107
funcname_builder=identity,
109108
eval_mode=false,
@@ -114,8 +113,7 @@ function test_coverage_edge_cases()
114113
e
115114
end
116115

117-
@test err isa ErrorException
118-
@test occursin("Test file", err.msg)
116+
@test err isa CTBase.Exceptions.IncorrectArgument
119117
@test occursin("not found", err.msg)
120118

121119
finally

test/suite/extensions/test_coverage_post_process.jl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ function test_coverage_post_process()
2222
e
2323
end
2424

25-
@test err isa ErrorException
25+
@test err isa CTBase.Exceptions.PreconditionError
2626
@test occursin("no .cov files", lowercase(err.msg))
2727
end
2828
end

test/suite/extensions/test_documenter_reference.jl

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ function test_documenter_reference()
7070
DR = DocumenterReference
7171

7272
@testset verbose = VERBOSE showtiming = SHOWTIMING "Invalid primary_modules input" begin
73-
@test_throws ErrorException CTBase.automatic_reference_documentation(
73+
@test_throws CTBase.Exceptions.IncorrectArgument CTBase.automatic_reference_documentation(
7474
CTBase.Extensions.DocumenterReferenceTag();
7575
subdirectory="ref",
7676
primary_modules=["invalid_string"], # String is not Module or Pair
@@ -386,7 +386,7 @@ function test_documenter_reference()
386386
)
387387

388388
# public=false, private=false should error
389-
@test_throws ErrorException CTBase.automatic_reference_documentation(
389+
@test_throws CTBase.Exceptions.IncorrectArgument CTBase.automatic_reference_documentation(
390390
CTBase.Extensions.DocumenterReferenceTag();
391391
subdirectory="ref",
392392
primary_modules=[DocumenterReferenceTestMod],

0 commit comments

Comments
 (0)