diff --git a/TODO6.md b/TODO6.md deleted file mode 100644 index 6b01a3f8..00000000 --- a/TODO6.md +++ /dev/null @@ -1,72 +0,0 @@ -# TODO6: Test Quality Review Against how_to_write_good_tests.md - -## A. Files Over 5,000 Lines (Rule 4) - -| File | Lines | Action | -|------|-------|--------| -| sdc/test/cpp/TestSdc.cc | 10,831 | Split into 2-3 files | -| search/test/cpp/TestSearchStaDesign.cc | 8,736 | Split into 2 files | -| liberty/test/cpp/TestLibertyStaBasics.cc | 6,747 | Split into 2 files | -| network/test/cpp/TestNetwork.cc | 5,857 | Split into 2 files | - -## B. Weak Assertions: (void) Casts (Rule 3) - -Replace `(void)result` with appropriate `EXPECT_*` assertions. - -| File | Test Name(s) | Count | -|------|-------------|-------| -| liberty/test/cpp/TestLibertyStaBasics.cc | TimingArcSetSense, TimingArcSetArcTo, +20 more | ~22 | -| liberty/test/cpp/TestLibertyClasses.cc | DefaultScaleFactors, SetIsClockCell, SetLevelShifterType, SetSwitchCellType | 5 | -| search/test/cpp/TestSearchStaDesign.cc | TagOperations, PathEndCmp, WnsSlackLess, MaxSkewCheckAccessors, MinPeriodCheckAccessors, +100 more | ~100+ | -| search/test/cpp/TestSearchStaInitB.cc | SearchTagCount2, SearchTagGroupCount2, SearchClkInfoCount2, +15 more | ~18 | -| search/test/cpp/TestSearchStaInit.cc | SigmaFactor, SetArcDelayCalc, SetParasiticAnalysisPts, SetTimingDerateGlobal | 4 | -| sdc/test/cpp/TestSdc.cc | ExceptionPathLessComparator, ClockPairLessOp, ClockSetCompare, PinClockPairLessDesign, PinPairHashConstruct | 5 | -| network/test/cpp/TestNetwork.cc | PinIdLessConstructor, NetIdLessConstructor, InstanceIdLessConstructor, PortIdLessConstructor, CellIdLessConstructor, PinSetCompare, NetSetCompare, AdapterCellAttributeMap, AdapterInstanceAttributeMap, AdapterPinVertexId, AdapterBusName | 13 | -| parasitics/test/cpp/TestParasitics.cc | PoleResidueBaseSetPiModel, PoleResidueBaseSetElmore, +11 more | 13 | -| dcalc/test/cpp/TestDcalc.cc | FindRoot (void)root casts | 3 | -| dcalc/test/cpp/TestFindRoot.cc | (void)root cast | 1 | -| power/test/cpp/TestPower.cc | PinActivityQuery (void)density/duty | 1 | -| spice/test/cpp/TestSpice.cc | VertexArrivalForSpice (void)arr | 1 | -| util/test/cpp/TestUtil.cc | RedirectFileAppendBegin (void)bytes_read | 1 | - -## C. Weak Assertions: SUCCEED() / EXPECT_TRUE(true) (Rule 9) - -| File | Test Name(s) | Count | -|------|-------------|-------| -| dcalc/test/cpp/TestDcalc.cc | TimingDmpCeffElmore, TimingDmpCeffTwoPole, TimingLumpedCap, TimingArnoldi, TimingUnit, GraphDelayCalcFindDelays, TimingCcsCeff, TimingPrima, IncrementalDelayWithDesign, SwitchDelayCalcMidFlow, +8 DesignDcalcTest, +6 GraphDelayCalc | ~24 | -| sdc/test/cpp/TestSdc.cc | CycleAcctingHashAndEqual, ClkNameLessInstantiation, ClockNameLessInstantiation | 3 | -| sdf/test/cpp/TestSdf.cc | WriteThenReadSdf, ReadSdfUnescapedDividers, +13 more | ~15 | -| network/test/cpp/TestNetwork.cc | ConstantNetsAndClear, LibertyLibraryIterator | 2 | -| verilog/test/cpp/TestVerilog.cc | StmtDestructor, InstDestructor | 2 | -| util/test/cpp/TestUtil.cc | LogEndWithoutLog, RedirectFileEndWithoutRedirect, StringDeleteCheckNonTmp, PrintConsoleDirect, StatsConstructAndReport, FlushExplicit, PrintErrorConsole, StringDeleteCheckRegular, PrintErrorConsoleViaWarn | 9 | - -## D. Load-only Tcl Tests (Rule 6) - -| File | Issue | -|------|-------| -| sdc/test/sdc_exception_intersect.tcl | write_sdc only, no report_checks/diff_files, .ok empty | -| sdc/test/sdc_exception_thru_complex.tcl | write_sdc only, no report_checks/diff_files, .ok empty | -| sdc/test/sdc_exception_override_priority.tcl | write_sdc only, no report_checks/diff_files, .ok empty | -| sdc/test/sdc_write_roundtrip_full.tcl | write_sdc/read_sdc only, no verification | -| liberty/test/liberty_equiv_cells.tcl | equiv_cells results stored but never verified | -| liberty/test/liberty_multi_lib_equiv.tcl | equiv results stored but never verified | - -## E. Orphan .ok Files (Rule 8) - -| File | Issue | -|------|-------| -| test/delay_calc.ok | No matching .tcl | -| test/min_max_delays.ok | No matching .tcl | -| test/multi_corner.ok | No matching .tcl | -| test/power.ok | No matching .tcl | -| test/power_vcd.ok | No matching .tcl | -| test/sdf_delays.ok | No matching .tcl | -| test/spef_parasitics.ok | No matching .tcl | - -## F. Inline Data File Creation (Rule 1) — Won't Fix - -The following use inline data by design (testing specific parser constructs not covered by checked-in libraries): -- liberty/test/cpp/TestLibertyStaCallbacks.cc — R9_/R11_ tests for specific liberty parser callbacks -- sdf/test/cpp/TestSdf.cc — Testing specific SDF constructs -- spice/test/cpp/TestSpice.cc — Testing CSV/SPICE parser with specific data patterns -- spice/test/*.tcl (8 files) — SPICE tests need inline model/subckt data (no checked-in SPICE models exist) diff --git a/bug_report_missing_write_gate_spice_cmd.md b/bug_report_missing_write_gate_spice_cmd.md deleted file mode 100644 index 657e351f..00000000 --- a/bug_report_missing_write_gate_spice_cmd.md +++ /dev/null @@ -1,36 +0,0 @@ -# Bug Report: Missing SWIG binding for `write_gate_spice_cmd` - -## Summary - -`write_gate_spice` Tcl command always fails with `invalid command name "write_gate_spice_cmd"` because the underlying C++ SWIG binding is not registered. - -## Details - -- **`write_gate_spice`** is defined as a Tcl proc in `spice/WriteSpice.tcl:138` -- It calls `write_gate_spice_cmd` at line 198 to invoke the C++ implementation -- **`write_gate_spice_cmd` is not registered** in any Tcl namespace (neither global nor `::sta::`) -- In contrast, `write_path_spice_cmd` IS correctly registered as `::sta::write_path_spice_cmd` - -## Reproduction - -```tcl -sta::sta -no_init -no_splash -exit /dev/stdin <<'EOF' -puts [info commands ::sta::write_path_spice_cmd] ;# returns ::sta::write_path_spice_cmd -puts [info commands ::sta::write_gate_spice_cmd] ;# returns empty -EOF -``` - -## Impact - -All `write_gate_spice` calls silently fail when wrapped in `catch`, or crash `sta` when called directly. The C++ implementation likely exists in `WriteSpice.cc` but the SWIG `.i` file is missing the binding declaration for `write_gate_spice_cmd`. - -## Affected Test Files - -The following test files had `write_gate_spice` catch blocks that always produced error output: -- `spice/test/spice_gate_advanced.tcl` -- `spice/test/spice_gate_cells.tcl` -- `spice/test/spice_gcd_gate.tcl` -- `spice/test/spice_gcd_path.tcl` -- `spice/test/spice_multipath.tcl` -- `spice/test/spice_subckt_file.tcl` -- `spice/test/spice_write_options.tcl` diff --git a/how_to_write_good_tests.md b/how_to_write_good_tests.md deleted file mode 100644 index 20302cf1..00000000 --- a/how_to_write_good_tests.md +++ /dev/null @@ -1,332 +0,0 @@ -# How to Write Good Tests for OpenSTA - -OpenSTA test review (2025-2026)에서 발견된 주요 품질 문제를 정리한 가이드. -향후 테스트 작성 및 리뷰 시 체크리스트로 활용할 것. - ---- - -## 1. Inline Data File Creation 금지 - -테스트 내에서 임시 파일을 생성하여 데이터를 만들지 말 것. -체크인된 테스트 데이터 파일(`test/nangate45/`, `test/sky130hd/` 등)을 사용해야 한다. - -**Bad:** -```cpp -// 임시 .lib 파일을 C++ 문자열로 직접 생성 -static const char *lib_content = R"( -library(test_lib) { - ... -} -)"; -FILE *f = fopen("/tmp/test.lib", "w"); -fprintf(f, "%s", lib_content); -fclose(f); -sta->readLiberty("/tmp/test.lib", ...); -``` - -**Good:** -```cpp -// 체크인된 실제 liberty 파일 사용 -LibertyLibrary *lib = sta->readLiberty( - "test/nangate45/Nangate45_typ.lib", - sta->cmdCorner(), MinMaxAll::min(), false); -ASSERT_NE(lib, nullptr); -``` - -**이유:** -- 임시 파일 정리 실패 시 테스트 환경 오염 -- 코드 리뷰 시 데이터와 로직이 혼재되어 가독성 저하 - ---- - -## 2. catch 사용 금지 - -Tcl 테스트에서 `catch` 블록으로 에러를 삼키지 말 것. -에러가 발생하면 테스트가 실패해야 한다. - -**Bad:** -```tcl -# 에러를 삼켜서 테스트가 항상 성공하는 것처럼 보임 -catch { report_checks -from [get_ports in1] } result -puts "PASS: report_checks" -``` - -**Good:** -```tcl -# 에러 발생 시 테스트 자체가 실패함 -report_checks -from [get_ports in1] -to [get_ports out1] -``` - -**catch가 허용되는 유일한 경우:** 에러 발생 자체를 검증하는 테스트 -```tcl -# 존재하지 않는 셀을 찾을 때 에러가 발생하는지 확인 -if { [catch { get_lib_cells NONEXISTENT } msg] } { - puts "Expected error: $msg" -} else { - puts "FAIL: should have raised error" -} -``` - -C++ 테스트에서도 동일한 원칙 적용: -```cpp -// Bad: 예외를 삼키는 것 -ASSERT_NO_THROW(some_function()); // 이것만으로 의미 없음 - -// Good: 실제 결과를 검증 -some_function(); -EXPECT_EQ(result, expected_value); - -// 예외 발생을 테스트하는 경우만 EXPECT_THROW 사용 -EXPECT_THROW(sta->endpoints(), Exception); -``` - ---- - -## 3. 테스트 당 최소 5개 이상의 유의미한 assertion - -단순 존재 확인(null 체크, 파일 존재 여부)만으로는 충분하지 않다. -각 테스트 케이스는 기능의 정확성을 검증하는 최소 5개 이상의 checker를 포함해야 한다. - -**Bad:** -```cpp -// 존재 확인만 하는 무의미한 테스트 -TEST_F(StaDesignTest, BasicCheck) { - EXPECT_NE(sta_, nullptr); // 1: null 아님 -- 당연함 - EXPECT_NE(sta_->network(), nullptr); // 2: null 아님 -- 당연함 - SUCCEED(); // 3: 아무것도 검증 안 함 -} -``` - -```tcl -# 파일 존재 여부만 확인하는 무의미한 Tcl 테스트 -write_verilog $out1 -if { [file exists $out1] && [file size $out1] > 0 } { - puts "PASS: output file exists" -} -``` - -**Good:** -```cpp -TEST_F(StaDesignTest, TimingAnalysis) { - Slack worst_slack; - Vertex *worst_vertex; - sta_->worstSlack(MinMax::max(), worst_slack, worst_vertex); - - EXPECT_NE(worst_vertex, nullptr); // 1: vertex 존재 - EXPECT_LT(worst_slack, 0.0f); // 2: slack 범위 확인 - EXPECT_GT(worst_slack, -1e6); // 3: 합리적 범위 - EXPECT_NE(sta_->graph()->pinDrvrVertex( // 4: graph 연결성 - sta_->network()->findPin("r1/D")), nullptr); - PathEndSeq ends = sta_->findPathEnds(...); - EXPECT_GT(ends.size(), 0u); // 5: path 존재 -} -``` - -```tcl -# 실제 타이밍 값을 검증하는 Tcl 테스트 -read_liberty test/nangate45/Nangate45_typ.lib -read_verilog examples/example1.v -link_design top -create_clock -name clk -period 10 {clk1 clk2 clk3} -report_checks -from [get_ports in1] -to [get_ports out1] -# .ok 파일의 diff로 slack, delay, path를 모두 검증 -``` - -**유의미한 assertion 예시:** -- 타이밍 값(slack, delay, slew) 범위 검증 -- 셀/포트/핀의 속성값 비교 -- 그래프 연결성 확인 -- 리포트 출력의 golden file diff -- 에러 조건에서의 예외 발생 확인 - ---- - -## 4. 너무 큰 .cc 파일 지양 - -단일 테스트 소스 파일은 **5,000줄 이하**를 유지할 것. -GCC는 대형 파일에서 `variable tracking size limit exceeded` 경고를 발생시키며, -이는 디버그 빌드의 품질을 저하시킨다. - -**실제 사례:** -- `TestLiberty.cc` (14,612줄) -> 3개 파일로 분할 -- `TestSearch.cc` (20,233줄) -> 4개 파일로 분할 - -**분할 기준:** -- 테스트 fixture 클래스 별로 분할 (서로 다른 SetUp/TearDown) -- 기능 영역별 분할 (class tests / Sta-based tests / callback tests) -- 의존성 수준별 분할 (standalone / Tcl required / design loaded) - -**CMakeLists.txt에서 매크로로 반복 제거:** -```cmake -macro(sta_cpp_test name) - add_executable(${name} ${name}.cc) - target_link_libraries(${name} OpenSTA GTest::gtest GTest::gtest_main ${TCL_LIBRARY}) - target_include_directories(${name} PRIVATE ${STA_HOME}/include/sta ${STA_HOME} ${CMAKE_BINARY_DIR}/include/sta) - gtest_discover_tests(${name} WORKING_DIRECTORY ${STA_HOME} PROPERTIES LABELS "cpp;module_liberty") -endmacro() - -sta_cpp_test(TestLibertyClasses) -sta_cpp_test(TestLibertyStaBasics) -sta_cpp_test(TestLibertyStaCallbacks) -``` - -**부수 효과:** 파일 분할은 빌드 병렬성도 향상시킨다. - ---- - -## 5. `puts "PASS: ..."` 출력 금지 (Tcl 테스트) - -Tcl 테스트의 합격/불합격은 `.ok` golden file과의 diff로 결정된다. -`puts "PASS: ..."` 는 테스트가 실패해도 항상 출력되므로 혼란만 초래한다. - -**Bad:** -```tcl -report_checks -puts "PASS: baseline timing" - -set_wire_load_model "large" -report_checks -puts "PASS: large wireload" -``` - -**Good:** -```tcl -report_checks - -set_wire_load_model "large" -report_checks -# .ok 파일 diff가 검증을 수행함 -``` - ---- - -## 6. Tcl 테스트는 독립적으로 실행 가능해야 함 - -하나의 `.tcl` 파일에 여러 독립 테스트를 넣지 말 것. -디버깅 시 특정 테스트만 실행할 수 없게 된다. - -**Bad:** -```tcl -# 하나의 파일에 10개 테스트가 모두 들어있음 -# 3번째 테스트를 디버깅하려면 1,2번을 먼저 실행해야 함 -read_liberty lib1.lib -puts "PASS: read lib1" -read_liberty lib2.lib -puts "PASS: read lib2" -# ... 반복 -``` - -**Good:** -```tcl -# liberty_read_nangate.tcl - Nangate45 라이브러리 읽기 및 검증 -read_liberty ../../test/nangate45/Nangate45_typ.lib -report_lib_cell NangateOpenCellLibrary/BUF_X1 -report_lib_cell NangateOpenCellLibrary/INV_X1 -# 하나의 주제에 집중하는 독립 테스트 -``` - -동일 liberty를 여러 번 로드하면 `Warning: library already exists` 경고가 발생하므로, -각 테스트 파일은 자체 환경을 구성해야 한다. - ---- - -## 7. Load-only 테스트 금지 - -파일을 읽기만 하고 내용을 검증하지 않는 테스트는 무의미하다. - -**Bad:** -```tcl -read_liberty ../../test/asap7/asap7sc7p5t_INVBUF_SLVT_TT_nldm_220122.lib.gz -puts "PASS: read ASAP7 INVBUF SLVT TT" -# 라이브러리를 읽기만 하고, 셀/포트/타이밍 아크를 전혀 검증하지 않음 -``` - -**Good:** -```tcl -read_liberty ../../test/asap7/asap7sc7p5t_INVBUF_SLVT_TT_nldm_220122.lib.gz -report_lib_cell asap7sc7p5t/INVx1_ASAP7_75t_SL -# .ok diff가 셀 정보, 타이밍 모델, 핀 방향 등을 검증 -``` - ---- - -## 8. Stale 주석 금지 - -커버리지 라인 번호, hit count 등 시간이 지나면 달라지는 정보를 주석에 넣지 말 것. - -**Bad:** -```tcl -# Targets: VerilogWriter.cc uncovered functions: -# writeInstBusPin (line 382, hit=0), writeInstBusPinBit (line 416, hit=0) -``` - -**Good:** -```tcl -# Test write_verilog with bus pins and bit-level connections -``` - ---- - -## 9. `EXPECT_TRUE(true)` / `SUCCEED()` 금지 (C++ 테스트) - -아무것도 검증하지 않는 assertion은 테스트가 아니다. - -**Bad:** -```cpp -TEST_F(SdcTest, ClkNameLess) { - ClkNameLess cmp; - (void)cmp; - EXPECT_TRUE(true); // "컴파일 되면 성공" -- 의미 없음 -} -``` - -**Good:** -```cpp -TEST_F(SdcTest, ClkNameLess) { - Clock *clk_a = makeClock("aaa", ...); - Clock *clk_b = makeClock("bbb", ...); - ClkNameLess cmp; - EXPECT_TRUE(cmp(clk_a, clk_b)); // "aaa" < "bbb" - EXPECT_FALSE(cmp(clk_b, clk_a)); // "bbb" < "aaa" is false - EXPECT_FALSE(cmp(clk_a, clk_a)); // reflexive -} -``` - ---- - -## 10. 코어 기능은 C++ 테스트로 - -Tcl 테스트는 SDC 명령어, 리포트 형식 등 사용자 인터페이스 검증에 적합하다. -delay calculation, graph traversal 등 코어 엔진 로직은 C++ 테스트로 작성해야 한다. - -| 적합 | C++ 테스트 | Tcl 테스트 | -|------|-----------|-----------| -| 예시 | delay calc, graph ops, path search | SDC commands, report formats, write_verilog | -| 장점 | 빠름, 디버거 연동, 세밀한 검증 | golden file diff, 실제 사용 시나리오 | -| 단점 | 셋업 코드 필요 | 느림, 디버깅 어려움 | - ---- - -## 11. Golden File(.ok) 관리 - -- 모든 Tcl 테스트는 대응하는 `.ok` 파일이 있어야 함 (orphan 방지) -- `.ok` 파일 없는 Tcl 테스트는 dead test -- 삭제하거나 `.ok` 생성 -- `.ok` 파일만 있고 `.tcl`이 없는 경우도 정리 대상 - ---- - -## 체크리스트 요약 - -테스트 PR 리뷰 시 다음을 확인: - -- [ ] 인라인 데이터 파일 생성 없음 (체크인된 테스트 데이터 사용) -- [ ] `catch` 블록 없음 (에러 테스트 목적 제외) -- [ ] 테스트 당 assertion 5개 이상 (null 체크만으로 불충분) -- [ ] 소스 파일 5,000줄 이하 -- [ ] `puts "PASS: ..."` 없음 -- [ ] `EXPECT_TRUE(true)` / `SUCCEED()` 없음 -- [ ] load-only 테스트 없음 (읽은 데이터를 반드시 검증) -- [ ] stale 라인 번호/커버리지 주석 없음 -- [ ] 코어 로직은 C++ 테스트로 작성 -- [ ] 대응하는 `.ok` 파일 존재 (Tcl 테스트) -- [ ] 각 테스트 파일은 독립 실행 가능