Repository navigation
Conversation
Several fixtures build their .tsfile name from a clock count alone. CTest runs cases in parallel processes, so two can read the same tick and then one removes the file the other is opening or reopening (E_ALREADY_EXIST / E_FILE_OPEN_ERR, seen as SEGFAULT on macOS). Append the process id and a per-process counter, the pattern the writer tests already use. Covers table_read_failure, table_field_filter, chunk_reader_resource, aligned_chunk_reader_bulk, aligned_field_filter_regression, masked_bulk_copy and timeseries_schema_codec.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1102 +/- ##
========================================
Coverage 66.93% 66.93%
========================================
Files 788 788
Lines 57536 57536
Branches 9297 9297
========================================
Hits 38509 38509
Misses 17358 17358
Partials 1669 1669 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
chunk_reader_resource_test.cc: PID headers are included only when… · Newprocess.h/unistd.hare currently included only under#ifdef ENABLE_THREADS, but this file… · New
What changed in this PR
This PR reduces flaky C++ tests by making temporary .tsfile names unique across parallel CTest processes, preventing cross-process collisions and intermittent open/remove failures.
Changes:
- Append process id and a per-process counter to test-generated
.tsfilenames. - Add platform-specific PID includes (
process.h/unistd.h) and atomics to support uniqueness.
| File | Description |
|---|---|
| cpp/test/reader/timeseries_schema_codec_test.cc | Adds PID + atomic counter suffix to generated names for cross-process uniqueness. |
| cpp/test/reader/table_read_failure_test.cc | Makes table_read_failure_*.tsfile names unique via PID + counter. |
| cpp/test/reader/table_field_filter_test.cc | Makes table_field_filter_*.tsfile names unique via PID + counter. |
| cpp/test/reader/chunk_reader_resource_test.cc | Extends filename generation to include PID + atomic counter (under thread-enabled build sections). |
| cpp/test/reader/block/masked_bulk_copy_test.cc | Makes masked_bulk_copy_*.tsfile names unique via PID + counter. |
| cpp/test/reader/aligned_field_filter_regression_test.cc | Makes regression test .tsfile names unique via PID + counter. |
| cpp/test/reader/aligned_chunk_reader_bulk_test.cc | Extends filename generation to include PID + atomic counter (under thread-enabled build sections). |
Review effort: Lite
| std::chrono::steady_clock::now().time_since_epoch().count()) + | ||
| ".tsfile"; | ||
| "_" + std::to_string(process_id) + "_" + | ||
| std::to_string(filename_counter.fetch_add(1)) + ".tsfile"; |
Comment on lines
+87
to
+88
| // CTest runs cases in parallel processes; two can seed from the same | ||
| // clock tick and then collide on the file name. |
Copilot stopped reviewing on behalf of
ColinLeeo due to an error
November 13, 2026 11:12
getpid()/_getpid() are used in the fixtures unconditionally, but the process.h/unistd.h block landed inside the ENABLE_THREADS guard in aligned_chunk_reader_bulk_test and chunk_reader_resource_test, which would break a build with threads disabled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Several test fixtures build their
.tsfilename from a clock count alone:CTest runs test cases in parallel processes, so two of them can read the same clock tick and generate the same name. One process then removes the file the other is about to open (or reopens it later), which surfaces as
E_ALREADY_EXISTfromTsFileWriter::open,E_FILE_OPEN_ERRfromWriteFile::open, or a SEGFAULT in a fixture that continues after the failed open.This has been flaking unrelated PRs (macOS 3.11, MSVC, macOS Release jobs) with failures in
TsFileReaderTest.*,TableFieldFilterTest.*,RowAndBatchParallelModes/TableFieldFilterTest.*.Change
Append the process id and a per-process counter to those names, the same pattern
tsfile_writer_test.ccandtsfile_writer_empty_chunk_regression_test.ccalready use:table_field_filter_test.cctable_read_failure_test.ccchunk_reader_resource_test.ccaligned_chunk_reader_bulk_test.ccaligned_field_filter_regression_test.ccblock/masked_bulk_copy_test.cctimeseries_schema_codec_test.cctsfile_reader_test.ccalready seeds fromstd::random_device, so it is left as is.Testing
Full C++ suite: 1528 tests passed;
spotless:checkclean.